Hi Laszlo, Thank you for valuable comments, I did not realize TimerLib should not be used at all for ShellPkg. I will remove -m option for now and then will replace with gRT time functions. > -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Monday, July 27, 2020 1:39 AM > To: vladimir.olovyanni...@broadcom.com; Zhichao Gao > <zhichao....@intel.com>; Ray Ni <ray...@intel.com> > Cc: devel@edk2.groups.io; Samer El-Haj-Mahmoud <Samer.El-Haj- > mahm...@arm.com>; Maciej Rabeda <maciej.rab...@linux.intel.com>; > Jiaxin Wu <jiaxin...@intel.com>; Siyuan Fu <siyuan...@intel.com>; Liming > Gao <liming....@intel.com>; Nd <n...@arm.com> > Subject: Re: [edk2-devel] [PATCH v4 0/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > 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. > > OK, > (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. OK, makes sense. > > (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). I read it, thanks. Now I understand the complications. > > > ... 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/HttpDynamicComman > d.inf] [X64] > > consumed by module > > > [ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicComman > d.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-F > > ormat > > 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".) OK, will do. > > 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). Thanks, will do that. > > > 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. Sure, totally makes sense. > > > 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 Thank you, Vladimir
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#63344): https://edk2.groups.io/g/devel/message/63344 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] -=-=-=-=-=-=-=-=-=-=-=-
