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

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

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*"
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"

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.  However, I
   would prefer the list of packages in Maintainers.txt be
   limited to the packages in that repo.  So the Readme.md
   in edk2-libc would point to Maintainers.txt in the edk2-libc
   repo for the packages in edk2-libc and provide a link to 
   Maintainers.txt in edk2 repo for the stewards.

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.

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, adding the
   edk2-libc repo 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.  There are no plans to move the
   EmulatorPkg or the OvmfPkg out of the edk2 repo.

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 do not think unit tests should have a dependency on libc.
   I recommend we port the OrderedCollectionLib to not depend
   on libc and remove it from the AppPkg.

   I would prefer to enter a separate BZ to work on this port.

Thanks,

Mike

> -----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
> 
>