Re: [edk2-devel] [PATCH v4 0/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

["Vladimir Olovyannikov" <vladimir.olovyannikov@broadcom.com>]

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 <lersek@redhat.com>
> Sent: Monday, July 27, 2020 1:39 AM
> To: vladimir.olovyannikov@broadcom.com; Zhichao Gao
> <zhichao.gao@intel.com>; Ray Ni <ray.ni@intel.com>
> Cc: devel@edk2.groups.io; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Maciej Rabeda <maciej.rabeda@linux.intel.com>;
> Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Liming
> Gao <liming.gao@intel.com>; Nd <nd@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.olovyannikov@broadcom.com>
> > Tested-By: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > Tested-By: Laszlo Ersek <lersek@redhat.com>
> > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> > Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Nd <nd@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 <lersek@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