Hi Vladimir,
On 07/23/20 22:50, Vladimir Olovyannikov via groups.io wrote:
> Signed-off-by: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>
> Tested-By: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>
> Tested-By: Laszlo Ersek <ler...@redhat.com>
> Cc: Zhichao Gao <zhichao....@intel.com>
> Cc: Maciej Rabeda <maciej.rab...@linux.intel.com>
> Cc: Jiaxin Wu <jiaxin...@intel.com>
> Cc: Siyuan Fu <siyuan...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Liming Gao <liming....@intel.com>
> Cc: Nd <n...@arm.com>
>
> This patchset introduces an http client utilizing EDK2 HTTP protocol, to
> allow fast image downloading from http/https servers.
> HTTP download speed is usually faster than tftp.
> The client is based on the same approach as tftp dynamic command, and
> uses the same UEFI Shell command line parameters. This makes it easy
> integrating http into existing UEFI Shell scripts.
> Note that to enable HTTP download, feature Pcd
> gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must be set to TRUE.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860
>
> PATCH v4 changes:
> Address comments based on Laszlo's testing:
> - Fix .uni help file missing "\r\n" before RETURNVALUES section;
> - delete the downloaded file in case of an error, unless
> -k ("keep bad") option is provided on command line.
>
> Allow download time measurement in seconds
> by providing -m parameter on command line.
(1) "Tested-by" tags cannot be carried forward on a patch if there are
any changes to the patch. If you update a patch and people would like to
have their T-b's on the patch, then they'll have to retest the patch. So
every time you update a patch, please drop the previously given
Tested-by tags from it.
(2) I was about to retest this patch, but I also compared it with the
previous version (v3). I think the "-m" option should not be added, for
now anyway. For two reasons:
- The patch is already large, and I think there has been no ShellPkg
reviewer / maintainer feedback so far. I think we shouldn't make the
patch even larger, with new features. "-m" looks like an addition that
can be done separately, once the core feature is upstream.
(I consider "-k" a bugfix on the other hand. More precisely, I
consider the new behavior *without "-k"* a bugfix. So I certainly
welcome that.)
- The other reason is that the "-m" feature seems to introduce a
TimerLib dependency. Depending on TimerLib in a shell application is
wrong, IMO; in particular because this application / dynamic command
is extremely useful, so some people might easily want to download a
pre-built binary, and run it on their system for whatever purposes.
But TimerLib is platform-dependent, and that would break this binary
portability.
For some more background, please refer to commit 7a141b1306f6
("ShellPkg: remove superfluous TimerLib resolution", 2018-02-13).
... In fact, upon re-reading the above commit, I'm realizing the current
patch could break the "ShellPkg.dsc" build, because the patch
(incorrectly) introduces a TimerLib dependency in a ShellPkg module, but
"ShellPkg.dsc" (correctly) does not resolve the TimerLib class to any
instance.
And it does break:
$ build -a X64 -b NOOPT -t GCC48 -p ShellPkg/ShellPkg.dsc
> ShellPkg/ShellPkg.dsc(...): error 4000: Instance of library class [TimerLib]
> is not found
> in
> [ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf] [X64]
> consumed by module
> [ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf]
One consequence of the above is that this patch would not pass a CI
build.
... Another reason this patch would not pass a CI build is a
"PatchCheck.py" failure:
> ShellPkg/DynamicCommand: add HttpDynamicCommand
> The commit message format is not valid:
> * 'Tested-By' should be 'Tested-by'
> * 'Tested-By' should be 'Tested-by'
> https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format
> The code passed all checks.
(Side comment: please use the clipboard for cutting and pasting feedback
tags from emails to commit messages. I have a keyboard shortcut for
inserting "Tested-by: Laszlo Ersek <ler...@redhat.com>", and so I know
for a fact that I never send an upper-case "By".)
I suggest running a personal CI build on github.com before submitting
the patch to the list (just open a pull request -- if it fails, you'll
get the error reports, and if it succeeds, then the mergify bot will
auto-close the successful personal build, without actually merging the
patch).
In summary, I suggest posting v5:
- with the Tested-by flags dropped (Samer and myself will have to
re-test),
- with the "-k" option preserved, but the "-m" option (and the
associated TimerLib dependency) removed.
Later on, I think "-m" could be added based on gRT->GetTime(), instead
of TimerLib.
Ray, Zhichao, when do you intend to review this patch? It does not make
much sense for Samer and myself to keep testing this patch if you're
going to start the review only later. The review feedback will probably
necessitate updates to the patch, which will invalidate Samer's testing
and mine. We've done a few rounds of testing; the patch certainly
deserves package maintainer review. Once no more changes looked
necessary from the reviewer side, I'd be happy to test the patch again.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#63293): https://edk2.groups.io/g/devel/message/63293
Mute This Topic: https://groups.io/mt/75754840/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
