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.kin...@intel.com>; > devel@edk2.groups.io > Cc: Carsey, Jaben <jaben.car...@intel.com>; Daryl > McDaniel (edk2-li...@mc2research.org) <edk2- > li...@mc2research.org>; leif.lindh...@linaro.org; Andrew > Fish (af...@apple.com) <af...@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- > d67f9dbdd...@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 > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39430): https://edk2.groups.io/g/devel/message/39430 Mute This Topic: https://groups.io/mt/31251207/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
