[edk2] Request to add new edk2-libc repository

[edk2] Request to add new edk2-libc repository

[Michael D Kinney]

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.  There is a single commit to
update the Readme.md and Maintainers.txt that scopes them
to this new edk2-libc repository.

Best regards,

Mike

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

[Laszlo Ersek]

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

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

[Michael D Kinney]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

On 04/24/19 11:50, Laszlo Ersek wrote:

> That's great, but there are two more sections in edk2-libc's
> Maintainers.txt that are not replated to edk2-libc's packages:

sorry, s/replated/related/

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

[Michael D Kinney]

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

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

[Laszlo Ersek]

On 04/25/19 22:52, Kinney, Michael D wrote:

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

OK.

> 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 have to be honest here: I've skimmed the above references, and I won't
be able to give them the attention they deserve. :(

Thanks,
Laszlo