Re: [edk2-devel] [edk2] Request to add new edk2-libc repository

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Laszlo,

Thanks for all your feedback.  Responses included below.

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, April 24, 2019 2:51 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Daryl
> McDaniel (edk2-lists@mc2research.org) <edk2-
> lists@mc2research.org>; leif.lindholm@linaro.org; Andrew
> Fish (afish@apple.com) <afish@apple.com>
> Subject: Re: [edk2-devel] [edk2] Request to add new edk2-
> libc repository
> 
> On 04/23/19 23:43, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > Many topics in here.  Please let me know if I missed
> anything.
> >
> > 1) I did use git filter-branch to extract the history
> of these
> >    packages.  The script I ran is shown below.  It
> results in
> >    a local repo in the directory edk2-filter that
> contains
> >    the 3 packages with complete history and the
> Maintainers,
> >    License, and Readme files.  The step remaining is to
> add a
> >    remote to the new edk2-libc repo and push the master
> branch
> >    in edk2-filter to edk2-libc.  I will add these
> details to
> >    the commit message.
> >
> > export PATHS_TO_KEEP="./AppPkg ./ StdLib
> ./StdLibPrivateInternalFiles ./Maintainers.txt ./License*
> ./Read*"
> 
> (probably typo in "./[space]StdLib")

Yes. A typo. I will fix.

> 
> > git clone https://github.com/tianocore/edk2.git edk2-
> filter
> > cd edk2-filter
> > git checkout master
> > git remote rm origin
> > git filter-branch -f --index-filter "git rm --ignore-
> unmatch --cached -qr -- . && git reset -q \$GIT_COMMIT --
> $PATHS_TO_KEEP" --prune-empty -- "master"
> 
> This is awesome! :) Please do include this description in
> a commit
> message (on a genuine, new commit) or in a text file
> (possibly part of
> Readme.md). Thanks!

I will put into Readme.md that explains where the content
originated.

> 
> > 2) There are 2 reviews.
> >
> > 2a)One to add new edk2-libc repo and populate with
> complete
> >    history and update the Maintainers.txt and Readme.md
> file.
> >
> >    https://github.com/mdkinney/edk2-libc
> >
> >    https://github.com/mdkinney/edk2-libc/commits/master
> >
> >    I think it makes sense to have a pointer to the edk2
> repo
> >    from the edk2-libc repo in the Readme.md.  I am ok
> with
> >    removing the stewards from the edk2-libc
> Maintainers.txt
> >    and pointing to Maintainers.txt in edk2 repo.
> 
> OK.
> 
> >    However, I
> >    would prefer the list of packages in Maintainers.txt
> be
> >    limited to the packages in that repo.
> 
> Yes, that's fine.
> 
> >    So the Readme.md
> >    in edk2-libc would point to Maintainers.txt in the
> edk2-libc
> >    repo for the packages in edk2-libc
> 
> That's OK.
> 
> >    and provide a link to
> >    Maintainers.txt in edk2 repo for the stewards.
> 
> That's great, but there are two more sections in edk2-
> libc's
> Maintainers.txt that are not replated to edk2-libc's
> packages:
> 
> - reporting security issues
> - releases
> 
> Both of those sections inherit the edk2 traits, and IMO
> that's not correct:
> 
> - In the TianoCore Bugzilla instance, the security
> process has
> disclaimed coverage for any and all issues found in libc
> (classifying
> them all as "normal bugs").
> 
> - Plus, the edk2 release process is not going to cover
> edk2-libc at all,
> at least until we invent new rules for edk2-libc.
> 
> For now, I think it would be OK if we just stated "TBD"
> or "work in
> progress" under both of these sections.

I agree.

> 
> >
> > 2b)Another review to remove the packages from the edk2
> repo
> >    and remove those packages from Maintainers.txt
> >    and Readme.md.  I did edit the commit to edk2 repo
> to make
> >    it smaller.  I have a V2 that does the remove in the
> first
> >    patch, and updates Maintainers.txt and Readme.md in
> a second
> >    patch.
> 
> Thanks. I guess my main point there was that even on the
> (now-)first
> patch, the diffstat should be included (the long list of
> files being
> removed). The contents of the files can be snipped.
> 
> $ git ls-files AppPkg/ StdLib/
> StdLibPrivateInternalFiles/ | wc
>    3647    3647  209360
> 
> Fewer than 4000 lines, and ~200KB in size. Even if it
> ends up doubled in
> the diffstat, that's not a huge email, and it helps with
> the review,
> IMO. (People can skim the list of files easily and catch
> something
> they'd like to keep.)

I will provide a V2 that preserves the list of files removed.

> 
> > 3) I understand your point about splitting actively
> maintained
> >    content into multiple repos and being able to use
> the
> >    history effectively to find the cause of a
> regression.
> >    The current plans for moving content are limited to
> the
> >    retiring packages that are no longer needed,
> 
> OK -- I've been collaborating on that (contributing to
> reviews and BZ
> discussions).
> 
> >    adding the edk2-libc repo
> 
> OK.
> 
> >    and moving content from the edk2 repo to the
> >    edk2-platforms repo.  We already have a number of
> platforms
> >    being maintained in edk2-platforms, so we will not
> be making
> >    it any worse by this move.
> 
> OK. As long as we're not making it any worse. I'll have
> to watch this
> space closely. I do understand that everything we
> preserve in edk2 will
> need active maintainership.
> 
> >    There are no plans to move the
> >    EmulatorPkg or the OvmfPkg out of the edk2 repo.
> 
> Thanks -- and please include ArmVirtPkg in the above list
> too.

Correct.  However, I would like to see if it is possible to
move ArmVirtPkg content into OvmfPkg, so OvmfPkg has the content
for running QEMU on all supported CPU archs.  But we can handle
this in a new discussion.

> 
> >
> > 4) I agree that the unit test for OrderedCollectionLib
> should
> >    be in the package that defines that library class.
> The
> >    SafeIntLib unit tests you point to is a better
> style.
> 
> I don't understand your second sentence here. Can you
> please elaborate
> on "better style"?

SafeIntLib demonstrates a unit test that runs without any
user input and verifies the result of each unit test and
provides test results in a standard format.

The OrderedCollectionLibTest provides a way for a developer
to manually test that lib class or generate a file from a 
Linux specific script with a set of commands to run from
an input file.  However, I do not see any functionality in
this test app to verify the results.  It appears to depend
on a developer manually inspecting the output.

Given that OrderedCollectionLibTest appears to be a manual
test requiring interaction, and since effort was made to 
implement this interactive utility, I would recommend this
remain in AppPkg in the edk2-libc repo so it is still
available for that use case.

As we enable automated unit tests in edk2, we can implement
a new automated unit test for the OrderedCollectionLib class.

Please see the following thread and branch for some ideas on
unit tests:

    https://edk2.groups.io/g/devel/message/39439

    https://github.com/tengfens/edk2-staging/tree/HBFA

I agree that writing unit tests must be made as simple as 
possible for developers to encourage everyone to implement
and submit unit tests for new content.  I look forward to
your feedback on that thread.

> 
> >    I do not think unit tests should have a dependency
> on libc.
> >    I recommend we port the OrderedCollectionLib to not
> depend
> >    on libc
> 
> I think this could make unit test development in general
> more expensive
> (in developer time).
> 
> For OrderedCollectionTest specifically, it would require
> us to reimplement:
> - fgets(),
> - getopt(),
> - strtol(),
> minimally. Those are LibC facilities that other unit
> tests could easily
> need (or benefit from), too.
> 
> ... I guess we could replace strtol() with
> ShellConvertStringToUint64(),
> from ShellLib.
> 
> >    and remove it from the AppPkg.
> >
> >    I would prefer to enter a separate BZ to work on
> this port.
> 
> I agree about tracking the port with a separate BZ, but
> what's the
> correct order of actions?
> 
> I'd like to avoid an interim when OrderedCollectionTest
> is absent from
> edk2. I'm OK if we move it around inside edk2 in
> arbitrary ways, in
> order to bridge the gap between removing AppPkg+StdLib,
> and porting the
> app to MdeModulePkg (with no StdLib dependencies).
> 
> Thanks,
> Laszlo
> 
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Tuesday, April 23, 2019 5:26 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> >> devel@edk2.groups.io
> >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Daryl
> >> McDaniel (edk2-lists@mc2research.org) <edk2-
> >> lists@mc2research.org>; leif.lindholm@linaro.org;
> Andrew
> >> Fish (afish@apple.com) <afish@apple.com>
> >> Subject: Re: [edk2-devel] [edk2] Request to add new
> edk2-
> >> libc repository
> >>
> >> Hello Mike,
> >>
> >> On 04/20/19 02:07, Kinney, Michael D wrote:
> >>> Hello,
> >>>
> >>> There were no objections to the following RFC to add
> >>> a new edk2-libc repository.
> >>>
> >>>     https://edk2.groups.io/g/devel/message/35211
> >>>
> >>> I have entered the following Feature Request Bugzilla
> >>>
> >>>
> https://bugzilla.tianocore.org/show_bug.cgi?id=1734
> >>>
> >>> I have posted a version of the edk2-libc repository
> for
> >>> review at the following location.  It includes all
> the
> >>> history for the three packages from the edk2
> >> repository.
> >>>
> >>>     https://github.com/mdkinney/edk2-libc
> >>>
> >>> Please review this branch.
> >>
> >> Can we document how this branch was extracted from the
> >> edk2 history? (I
> >> assume some form of branch rewriting.)
> >>
> >> If it's documented already, then I apologize for
> missing
> >> it.
> >>
> >>> There is a single commit to update the Readme.md and
> >> Maintainers.txt
> >>> that scopes them to this new edk2-libc repository.
> >>
> >> So it seems that said single commit (7e1bdd700213,
> "edk2-
> >> libc: Reduce
> >> scope of Readme.md and Maintainer.txt", 2019-04-19) is
> >> the only manually
> >> written one (not a result of branch rewriting). Is
> that
> >> right?
> >>
> >> ... Possible improvements for this commit:
> >>
> >> - Can we not duplicate the "Tianocore Stewards"
> section?
> >> Perhaps we can
> >>   include a pointer to the edk2 "Maintainers.txt"
> file.
> >>
> >> - "Responsible Disclosure, Reporting Security Issues":
> >> according to
> >>
> >>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1>,
> >> no issues
> >>   found under StdLib will be classified as security
> >> issues. I think we
> >>   should reflect that decision here.
> >>
> >> - "EDK II Releases": I think if we split off StdLib,
> then
> >> "releases"
> >>   will have to be defined from scratch. (But see my
> >> general objection
> >>   (1) below, anyway.)
> >>
> >> ----*----
> >>
> >> I've now re-read my comments under the RFC:
> >>
> >>   [edk2] [RFC v2] Proposal to add edk2-libc
> >>
> >>   https://lists.01.org/pipermail/edk2-devel/2019-
> >> January/035341.html
> >>   https://edk2.groups.io/g/devel/message/35321
> >>   http://mid.mail-archive.com/578c1f6c-945e-2f00-0cb4-
> >> d67f9dbdd50e@redhat.com
> >>
> >> There I wrote,
> >>
> >>> I'm not sure how closely the StdLib internals are
> tied
> >> to day-to-day
> >>> changes in core edk2; that is, whether we should keep
> >> those histories
> >>> interlinked. That's something for the StdLib
> >> maintainers to evaluate.
> >>> Personally I don't remember many StdLib changes, so
> >> there seems to be
> >>> a genuine separation that supports the new repo idea.
> >>
> >> I'm not going back on those specific thoughts, but I'd
> >> like to voice my
> >> disagreement on two points, one general and one
> specific.
> >>
> >> (1) My general objection is that this change seems to
> set
> >> a precedent
> >>     for fragmenting the edk2 repository into multiple
> >> repositories. I'm
> >>     opposed to that. I'm *now* seeing the removal of
> >> StdLib as an action
> >>     for establishing prior art while it doesn't hurt
> in
> >> practice, and
> >>     then using it as "past evidence" in support of
> >> splitting off more
> >>     packages and modules. While I don't particularly
> mind
> >> StdLib, I
> >>     definitely object to such a *trend*. (When I last
> >> commented on the
> >>     RFC, in January, I didn't expect it to become a
> >> trend. I do worry
> >>     about it now.)
> >>
> >> (2) My specific objection is that
> >> "Applications/OrderedCollectionTest"
> >>     is a unit test application for MdePkg's
> >> OrderedCollectionLib class.
> >>     This application has two relevant traits:
> >>
> >>     (2a) it depends on stdio for consuming input and
> >> producing output
> >>          (please see the commit message on
> 424d84556d4d,
> >> "AppPkg:
> >>          introduce OrderedCollectionTest", 2014-08-
> 12),
> >>
> >>     (2b) it must be in sync with the
> OrderedCollectionLib
> >> class, and the
> >>          (main) OrderedCollectionLib instance(s), at
> all
> >> times.
> >>
> >>     Due to (2b), I don't think this application should
> be
> >> removed from
> >>     the core edk2 repository (it's a validation tool).
> >> And, wrt. (2a), I
> >>     wouldn't like to give up the option of writing
> test
> >> apps /
> >>     "validators" that consume LibC -- the standard C
> >> library APIs allow
> >>     contributors to focus on the interfaces and tasks
> >> they actually want
> >>     to test.
> >>
> >>     I believe that, for
> >> "Applications/OrderedCollectionTest", it should
> >>     be "sort of" OK to split off StdLib; given that
> the
> >> application
> >>     assumes that StdLib "just works", and StdLib is
> not
> >> the app's main
> >>     focus. The standard C interfaces are specified
> >> separately
> >>     (independently of edk2), and so
> OrderedCollectionTest
> >> can be written
> >>     against ISO C, and we can expect users to make
> "some
> >> version" of
> >>     StdLib available through PACKAGES_PATH.
> >>
> >>     The same is not true of the
> >>     OrderedCollectionTest<->OrderedCollectionLib
> >> connection, which is
> >>     why I think the app itself should remain in core
> >> edk2. Perhaps we
> >>     should move the app under
> "MdeModulePkg/Application"
> >> first.
> >>
> >> I'm not sure about the validation role of the other
> apps
> >> under AppPkg.
> >> For example, "AppPkg/Applications/Sockets" used to be
> >> whole-sale helpful
> >> for SNP driver testing. However, I agree it is
> different:
> >> first because
> >> we now have HTTP boot over both IPv4 and IPv6, which
> is a
> >> good way to
> >> test TCP and everything below, and second because an
> SNP
> >> driver again
> >> implements standardized interfaces (namely from the
> UEFI
> >> spec), so an
> >> external project such as the UEFI SCT can be used to
> >> check SNP drivers.
> >>
> >> However, lib classes are entirely internal to edk2,
> and
> >> so if an app
> >> exists to validate instances of a given lib class,
> then
> >> the app too
> >> should stay within edk2.
> >>
> >> ----*----
> >>
> >> BTW... it now occurs to me that once in the past you
> >> referred to a unit
> >> test suite for SafeIntLib:
> >>
> >>   https://lists.01.org/pipermail/edk2-devel/2018-
> >> February/021551.html
> >>   http://mid.mail-
> >>
> archive.com/E92EE9817A31E24EB0585FDF735412F5B896820A@ORSM
> >> SX113.amr.corp.intel.com
> >>
> >> In my opinion, that application too should be brought
> >> into core edk2. It
> >> should share a common git history with the SafeIntLib
> >> class, and the
> >> (main) SafeIntLib instance.
> >>
> >>
> >> Sorry about the wall of text; in summary, I'd like to
> >> preserve
> >> OrderedCollectionTest in core edk2, and I'd like to
> speak
> >> out very
> >> clearly against setting a trend for fragmenting edk2
> into
> >> a multitude of
> >> sub-repositories. I don't mind StdLib and AppPkg in
> >> isolation.
> >>
> >> Thanks,
> >> Laszlo
> >>
> >> 
> >