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

["Laszlo Ersek" <lersek@redhat.com>]

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

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

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

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

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

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

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