RFC for Edk2-Library

RFC for Edk2-Library

[Sean]

[-- Attachment #1: Type: text/plain, Size: 5515 bytes --]

RFC  Edk2-Library creation

Create a new tianocore owned repository to host a python library package in support of UEFI development.  This package will allow easy sharing of python library code to facilitate reuse.  Inclusion of this package and dependency management should be managed using Pip/Pypi.   To start this is a supplemental package and is not required to be used for edk2 builds.

 

Examples of content here

* Edk2 file type parsing
* UEFI structure encode/decode in python
* Packaging tools (Capsules generation, signing, INF gen, Cat gen)
* TPM support code
* Potentially move content from basetools/source/python/common/*
* No command line tools/interface

Maintainers

* Sean Brogan
* Bret Barkelew
* Placeholder for existing maintainer from the basetools

License

* BSD + Patent (edk2 aligned)

Contribution process and issue tracking

* Follow Github PR process for contributions and issue tracking
* Contributor forks repo in github
* Contributor creates branch for work
* Contributor updates release notes to indicate change (if necessary)
* Contributor submits PR to master branch of tianocore/Edk2-Library repo
* Review feedback is given in PR
* Python Tests are run on the repo (new contributions need unit tests)
* Python Style (flake8) must pass
* All review feedback must be completed, maintainers approved, and tests run successfully before PR is *squash merged* into master

Documentation

* Use Github IO documentation/wiki hosting
* Example content

                                                              i.      https://microsoft.github.io/mu/dyn/mu_pip_python_library/developing/ ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmicrosoft.github.io%2Fmu%2Fdyn%2Fmu_pip_python_library%2Fdeveloping%2F&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=fKIb1Pfj4AqoGVOWudcyFMxMypJk%2FHTts9aMxZ8HukI%3D&reserved=0 )

                                                            ii.     https://microsoft.github.io/mu/dyn/mu_pip_python_library/publishing/ ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmicrosoft.github.io%2Fmu%2Fdyn%2Fmu_pip_python_library%2Fpublishing%2F&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=QJMUMB1hIusaRVJhgsi9kF9KIbgdhS0WRnIXVkGeBCM%3D&reserved=0 )

* Readme at root of repo
* Example: https://github.com/Microsoft/mu_pip_python_library ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Fmu_pip_python_library&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=DiuPnNMagvYYf0XXxDycSVHqijBBcDT0fXHzVY9U6%2Fw%3D&reserved=0 )

CI Builds

* CI build process using dev ops
* Validation is done thru build process
* Release publication done thru manual CI Build
* Examples from Mu-Python-Library
* Windows CI - https://dev.azure.com/projectmu/mu%20pip/_build?definitionId=13 ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Fprojectmu%2Fmu%2520pip%2F_build%3FdefinitionId%3D13&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=jQCCZo2u8JVisCchOoMLSgKJbG3YEk%2FG1JP9fI4g2JY%3D&reserved=0 )
* Linux CI - https://dev.azure.com/projectmu/mu%20pip/_build?definitionId=12 ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Fprojectmu%2Fmu%2520pip%2F_build%3FdefinitionId%3D12&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=CILlTdeEGpsi%2BCQNiZdIqd2Vt2RQV6L3qjz2rWEARYE%3D&reserved=0 )
* Publishing - https://dev.azure.com/projectmu/mu%20pip/_build?definitionId=16 ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Fprojectmu%2Fmu%2520pip%2F_build%3FdefinitionId%3D16&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=hkpzn9P6RBjduYBWjN56Y2qRBWyOG32mFkP%2BwtY7KBQ%3D&reserved=0 )

Release

* Release to Pypi as Edk2-Library for easy usage in product environment
* Versioned follows: Aa.bb.cc.dd
* AA == Major version.  Changes don’t need to be backward compatible
* BB == Minor version.  Significant new features.  Backward compatibility maintained
* CC == Bug fix/patch/small optional feature
* DD == build/Release version. 
* Package on Pypi will be owned by Tianocore group
* Example for mu-python-library: https://pypi.org/project/mu-python-library/ ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpypi.org%2Fproject%2Fmu-python-library%2F&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=5nTb93dDnAo%2FsWyfv1zYEIkJF8L58YY5P3BkzKi4Ivc%3D&reserved=0 )

Other Notes

* Only support Python 3 (prefer 3.7+)
* This was discussed on the edk2 design meetings (4/18) https://edk2.groups.io/g/devel/files/Designs/2019/0418 ( https://edk2.groups.io/g/devel/files/Designs/2019/0418 )
* There will be one more RFC for another python package/repo for Build environment. 

 

Timeline

* RFC open for comment thru 5/21/2019

[-- Attachment #2: Type: text/html, Size: 24110 bytes --]

Re: [edk2-devel] RFC for Edk2-Library

[Laszlo Ersek]

On 05/07/19 23:35, Sean via Groups.Io wrote:
> RFC  Edk2-Library creation
>
> Create a new tianocore owned repository to host a python library
> package in support of UEFI development.  This package will allow easy
> sharing of python library code to facilitate reuse.  Inclusion of this
> package and dependency management should be managed using Pip/Pypi. To
> start this is a supplemental package and is not required to be used
> for edk2 builds.

[1]

> Examples of content here
>
> * Edk2 file type parsing
> * UEFI structure encode/decode in python
> * Packaging tools (Capsules generation, signing, INF gen, Cat gen)
> * TPM support code
> * Potentially move content from basetools/source/python/common/*

[2]

> * No command line tools/interface
>
> Maintainers
>
> * Sean Brogan
> * Bret Barkelew
> * Placeholder for existing maintainer from the basetools
>
> License
>
> * BSD + Patent (edk2 aligned)
>
> Contribution process and issue tracking
>
> * Follow Github PR process for contributions and issue tracking
> * Contributor forks repo in github
> * Contributor creates branch for work
> * Contributor updates release notes to indicate change (if necessary)
> * Contributor submits PR to master branch of tianocore/Edk2-Library
>   repo
> * Review feedback is given in PR
> * Python Tests are run on the repo (new contributions need unit tests)
> * Python Style (flake8) must pass
> * All review feedback must be completed, maintainers approved, and
>   tests run successfully before PR is *squash merged* into master

The sentences

[1] "To start this is a supplemental package and is not required to be
     used for edk2 builds."

[2] "Potentially move content from basetools/source/python/common/*"

foreshadow that such a code movement might happen down the road, and the
external package could become a requirement for building edk2.

That step would mean the following:

(a) Edk2 would not remain buildable from a single command

    git clone --recurse-submodules

    Building edk2 would require GNU/Linux users to start tracking
    packages with "pip", which is independent of any given distro's own
    package management, and may cause conflicts if not used carefully:

      https://developer.fedoraproject.org/tech/languages/python/pypi-installation.html

    This requirement on "pip" would only go away once the external
    python dependencies were packaged for at least the larger GNU/Linux
    distros.

(b) Edk2 users running into build problems related to the external
    python dependencies would have to contribute through a github-only
    workflow. That's not a deal-breaker per se -- if we want to
    contribute to other edk2 dependencies, such as OpenSSL or nasm, we
    also have to conform to their specific development models, clearly.

    However, "squash merge" is a catastrophically bad development model,
    and I'd object to introducing a new edk2 build dependency that
    followed that model.

    (There are other issues with the GitHub.com development workflow, as
    discussed elsewhere, but "squash merge" takes the cake.)

Problem (a) would be solvable in the long term (through collaboration
with distro maintainers, i.e. downstream packaging), but problem (b)
would not be. Thus I'm fine with the proposal, in its current form, only
if the new package is 100% an addition on top of edk2, even in the long
term, and not in any part intended as a future replacement for current
edk2 functionality.

Thanks,
Laszlo

Re: [edk2-devel] RFC for Edk2-Library

[Michael D Kinney]

Hello,

It is difficult to tell if the repo name edk2-library
is for firmware or tools, so I recommend we work on a
name that clearly identifies that this repo is related
to tools.

For the pip dependencies, is the concern that a platform
that depends on these tools will not be buildable without
running a "pip install" command that pulls content from the
network?  We already have to pull content to get the sources
and potentially other dependent tools (NASM, iASL, openssl).

Can we limit the initial scope to tools that layer on top
of edk2, and a different future RFC would be required if 
there is a proposal for the edk2 repo to depend on another
repo?  If we accept this limited scope, are there still 
concerns about initially using squash commits for changes
to these tools.  Is there a way for GitHub to support both
squash commits for some PRs and preserve a patch series for
other PRs?
 
Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Laszlo Ersek
> Sent: Wednesday, May 8, 2019 2:56 AM
> To: devel@edk2.groups.io; sean.brogan@microsoft.com
> Subject: Re: [edk2-devel] RFC for Edk2-Library
> 
> On 05/07/19 23:35, Sean via Groups.Io wrote:
> > RFC  Edk2-Library creation
> >
> > Create a new tianocore owned repository to host a
> python library
> > package in support of UEFI development.  This package
> will allow easy
> > sharing of python library code to facilitate reuse.
> Inclusion of this
> > package and dependency management should be managed
> using Pip/Pypi. To
> > start this is a supplemental package and is not
> required to be used
> > for edk2 builds.
> 
> [1]
> 
> > Examples of content here
> >
> > * Edk2 file type parsing
> > * UEFI structure encode/decode in python
> > * Packaging tools (Capsules generation, signing, INF
> gen, Cat gen)
> > * TPM support code
> > * Potentially move content from
> basetools/source/python/common/*
> 
> [2]
> 
> > * No command line tools/interface
> >
> > Maintainers
> >
> > * Sean Brogan
> > * Bret Barkelew
> > * Placeholder for existing maintainer from the
> basetools
> >
> > License
> >
> > * BSD + Patent (edk2 aligned)
> >
> > Contribution process and issue tracking
> >
> > * Follow Github PR process for contributions and issue
> tracking
> > * Contributor forks repo in github
> > * Contributor creates branch for work
> > * Contributor updates release notes to indicate change
> (if necessary)
> > * Contributor submits PR to master branch of
> tianocore/Edk2-Library
> >   repo
> > * Review feedback is given in PR
> > * Python Tests are run on the repo (new contributions
> need unit tests)
> > * Python Style (flake8) must pass
> > * All review feedback must be completed, maintainers
> approved, and
> >   tests run successfully before PR is *squash merged*
> into master
> 
> The sentences
> 
> [1] "To start this is a supplemental package and is not
> required to be
>      used for edk2 builds."
> 
> [2] "Potentially move content from
> basetools/source/python/common/*"
> 
> foreshadow that such a code movement might happen down
> the road, and the
> external package could become a requirement for building
> edk2.
> 
> That step would mean the following:
> 
> (a) Edk2 would not remain buildable from a single
> command
> 
>     git clone --recurse-submodules
> 
>     Building edk2 would require GNU/Linux users to start
> tracking
>     packages with "pip", which is independent of any
> given distro's own
>     package management, and may cause conflicts if not
> used carefully:
> 
> 
> https://developer.fedoraproject.org/tech/languages/pytho
> n/pypi-installation.html
> 
>     This requirement on "pip" would only go away once
> the external
>     python dependencies were packaged for at least the
> larger GNU/Linux
>     distros.
> 
> (b) Edk2 users running into build problems related to
> the external
>     python dependencies would have to contribute through
> a github-only
>     workflow. That's not a deal-breaker per se -- if we
> want to
>     contribute to other edk2 dependencies, such as
> OpenSSL or nasm, we
>     also have to conform to their specific development
> models, clearly.
> 
>     However, "squash merge" is a catastrophically bad
> development model,
>     and I'd object to introducing a new edk2 build
> dependency that
>     followed that model.
> 
>     (There are other issues with the GitHub.com
> development workflow, as
>     discussed elsewhere, but "squash merge" takes the
> cake.)
> 
> Problem (a) would be solvable in the long term (through
> collaboration
> with distro maintainers, i.e. downstream packaging), but
> problem (b)
> would not be. Thus I'm fine with the proposal, in its
> current form, only
> if the new package is 100% an addition on top of edk2,
> even in the long
> term, and not in any part intended as a future
> replacement for current
> edk2 functionality.
> 
> Thanks,
> Laszlo
> 
> 

Re: [edk2-devel] RFC for Edk2-Library

[Laszlo Ersek]

On 05/09/19 20:06, Kinney, Michael D wrote:
> Hello,
>
> It is difficult to tell if the repo name edk2-library is for firmware
> or tools, so I recommend we work on a name that clearly identifies
> that this repo is related to tools.

Good idea.

> For the pip dependencies, is the concern that a platform that depends
> on these tools will not be buildable without running a "pip install"
> command that pulls content from the network?

Almost. Not exactly.

First, the concern is not specific to any given platform. My
understanding is that the extraction would target, in the longer term,
general BaseTools functionality. That would impact all platforms that
consume & build against edk2.

Second, it's not the network access that's concerning per se. It's the
compatibility / interoperation with any given Linux distribution's own
(native) package management system. Such a package management system is
generally not used on Windows, therefore Windows users are accustomed to
installing software packages from a boatload of vendors. This is not the
case on Linux distros -- your distro vendor offers a curated set of
packages, such that everything interoperates with everything (ideally),
there are no file conflicts, version dependencies are handled
automatically (e.g. if you install a high-level package, its
dependencies are pulled in automatically), and so on.

Clearly the distro vendor does not *author* all this software (although
they are strongly encouraged to contribute to the upstream software
projects they package and ship). The distro vendor is responsible for
the integration of all these packages into a consistent OS, into
consistent feature sets, and so on.

"pip" and similar tools are generally unfit for this approach, because
they implement a parallel package management system, to my
understanding. If they are carefully used in a user's home directory,
they might work. If packages were installed with "pip" system-wide, they
would almost certainly break (conflict with) the distro-provided
packages.

Therefore, when users would like to get a new piece of software
packaged, or an existent package refreshed (to a more recent upstream
version), they file a feature requst with their distro vendor (unless
the distro vendor already tracks the upstream project closely and
performs periodic rebases anyway). Then the distro vendor ships a new
(or refreshed) package, again nicely integrated with the rest of the
system.

> We already have to pull content to get the sources and potentially
> other dependent tools (NASM, iASL, openssl).

Yes, these are good examples to demonstrate the difference. Consider
NASM. If a Windows user would like to install NASM, they google "NASM
for Windows", then go to:

https://www.nasm.us/pub/nasm/releasebuilds/2.14.02/win64/

download the ZIP file or EXE file, and install it.

In comparison, a Fedora user runs

  dnf install nasm

and they get the package from the Fedora package repository. A Debian
user would run

  apt-get install nasm

and they would get the package (which would be entirely independent of
Fedora's) from the Debian package repository.

The various *BSDs would run a variant of

  pkg_add nasm

I believe.

The same applies to iASL.


OpenSSL is a special case. It is different from NASM and iASL. NASM and
iASL are native (hosted) applications, so any Linux distro can easily
build them (for itself), and the binaries can be shipped to users, ready
to run. But for the purposes of edk2, OpenSSL is not needed as a native
(hosted) utility, like NASM and iASL -- it is needed as a *cross-built*
static library, targeting the firmware architecture (not the host
architecture). Unfortunately, OpenSSL (the upstream project) does not
fully support this use case yet, therefore Linux distros cannot just
cross-compile OpenSSL for edk2, and offer static library packages. This
is why upstream edk2 consumes OpenSSL through a git submodule, in source
form.

*However*: conceptually, there is no difference. In Fedora and RHEL, we
don't build OVMF against upstream OpenSSL. We build it against the
downstream OpenSSL source code. So ultimately there is no difference in
the distro vendor's approach -- the sources and the binaries come from
the distro vendor.


So what follows from this? It means that, for using BaseTools on a Linux
distro, users will have to install some extra Python packages, in
addition to NASM and iASL, from their vendor's repository. In other
words, the "edk2-library" (or "edk2-tools") project should do upstream
releases, so that Linux distros can package them in their native package
systems. And some collaboration between upstream and downstreams is
expected for this. (Exactly as it occurs with the iASL and NASM upstream
projects.)

> Can we limit the initial scope to tools that layer on top of edk2, and
> a different future RFC would be required if there is a proposal for
> the edk2 repo to depend on another repo?

Yes, that would be very nice.

> If we accept this limited scope, are there still concerns about
> initially using squash commits for changes to these tools.

Yes, I still have the same concern -- although it's a milder concern,
dependent on how long squash merges are tolerated.

Assume that a second, separate RFC gets posted down the road, let's say
a year in, which proposes to replace the in-line component FooBar of
BaseTools, with the external project "edk2-library" (or "edk2-tools").
At that point, the git history accrued thus far, in edk2-tools, would
suddenly be inherited by all BaseTools users. And that history wouldn't
be really helpful.

For an example, we need not look past the edk2 repository itself. Let's
say I'd like to learn about the implementation history of the
HandleProtocol() boot service, in edk2. So I run:

$ git blame master -- MdeModulePkg/Core/Dxe/Hand/Handle.c

and I look up the CoreHandleProtocol() function, and find this:

> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  931) EFI_STATUS
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  932) EFIAPI
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  933) CoreHandleProtocol (
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  934)   IN EFI_HANDLE       UserHandle,
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  935)   IN EFI_GUID         *Protocol,
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  936)   OUT VOID            **Interface
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  937)   )
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  938) {
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  939)   return CoreOpenProtocol (
> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c (qhuang8          2008-07-24 02:54:45 +0000  940)           UserHandle,
> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c (qhuang8          2008-07-24 02:54:45 +0000  941)           Protocol,
> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c (qhuang8          2008-07-24 02:54:45 +0000  942)           Interface,
> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c (qhuang8          2008-07-24 02:54:45 +0000  943)           gDxeCoreImageHandle,
> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c (qhuang8          2008-07-24 02:54:45 +0000  944)           NULL,
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  945)           EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  946)           );
> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c (yshang1          2007-07-04 10:51:54 +0000  947) }

Interesting, it seems like the function was originally added in commit
28a00297189c3, and then the argument list was modified in commit
022c6d45ef786, a year later. So let's check out the rationale for that
argument list update:

$ git show --color 022c6d45ef786

Well... The commit message says, "Code Scrub for Dxe Core". That's all.
We also learn it comes from SVN revision 5560. And the diffstat for the
patch is:

>  38 files changed, 2499 insertions(+), 2504 deletions(-)

It is not overly helpful.

Clearly, CoreHandleProtocol() is a super-robust, super-mature function,
so we don't really care for its development history today.

I believe the same would not apply to the "edk2-library" (or
"edk2-tools") project. When it replaced the in-line component FooBar of
BaseTools, it's plausible it would be suddenly exposed to a larger
userbase. New issues would be encountered, and people might want to look
at the git history -- if for nothing else, then rationale (commit
messages) on small parts of the code.


I can also name more recent examples. I'm not a frequent BaseTools
contributor, but it has happened occasionally. (I've got 41 patches in
the edk2 git history that touched BaseTools/, as of commit
0a506fc7ab8b.)

* For <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>, I pushed
  5+1 patches:

    67983484a443 BaseTools/footer.makefile: expand BUILD_CFLAGS last for C files too
    03252ae287c4 BaseTools/header.makefile: remove "-c" from BUILD_CFLAGS
    b8a661702643 BaseTools/Source/C: split "-O2" to BUILD_OPTFLAGS
    b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS from the caller
    81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS from the caller
    +
    aa4e0df1f0c7 BaseTools/VfrCompile: honor EXTRA_LDFLAGS

* For <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>, I pushed 26
  patches:

    8ff122119941 EmulatorPkg: require GCC48 or later
    8d7cdfae8cb8 OvmfPkg: require GCC48 or later
    fd158437dcce Vlv2TbltDevicePkg: assume GCC48 or later
    7a9dbf2c94d1 BaseTools/Conf/tools_def.template: drop ARM/AARCH support from GCC46/GCC47
    48e64498c961 BaseTools/tools_def.template: fix up LF-only line terminator
    7381a6627a66 BaseTools/tools_def.template: strip trailing whitespace
    9bbf156faaad BaseTools/tools_def.template: remove GCC48_IA32_X64_DLINK_COMMON dead-end
    3c5613c5932c BaseTools/tools_def.template: remove GCC47 leaf definitions
    fc87b8d7f411 BaseTools/tools_def.template: propagate loss of GCC47 references
    91a67e0f111e BaseTools/tools_def.template: remove GCC47 documentation
    0f234fb8a662 BaseTools/tools_def.template: remove GCC46 leaf definitions
    83a8f313884a BaseTools/tools_def.template: propagate loss of GCC46 references
    be359fa7ceec BaseTools/tools_def.template: remove GCC46 documentation
    1458af0cbce0 BaseTools/tools_def.template: remove GCC45 leaf definitions
    024576896d42 BaseTools/tools_def.template: propagate loss of GCC45 references
    3e77d20f5cb3 BaseTools/tools_def.template: remove GCC45 documentation
    e046dc60fb89 BaseTools/tools_def.template: remove GCC44 leaf definitions
    38c570efede0 BaseTools/tools_def.template: propagate loss of GCC44 references
    383d29096846 BaseTools/tools_def.template: rename GCC44_ALL_CC_FLAGS to GCC48_ALL_CC_FLAGS
    0db91daf5228 BaseTools/tools_def.template: eliminate GCC44_IA32_X64_DLINK_FLAGS
    84d21abf4e36 BaseTools/tools_def.template: rename GCC44_IA32_X64_DLINK_COMMON to GCC48_IA32_X64_DLINK_COMMON
    5c6ccd53244b BaseTools/tools_def.template: remove comment about GCC44 + LzmaF86Compress
    3bc65326d6ed BaseTools/tools_def.template: remove GCC44 documentation
    f7282023e758 ArmPkg/ArmSoftFloatLib: drop build flags specific to GCC46/GCC47
    300b8c5f150c CryptoPkg/BaseCryptLib: drop build flags specific to GCC44
    7423ba9d499b Revert "MdePkg: avoid __builtin_unreachable() on GCC v4.4"

  (Note, from this list, 7a9dbf2c94d1 was authored by Ard, I just
  included it at the right spot.)

All of these patch series were carefully structured, and carefully
documented (in the commit messages). I would have been devastated, had
they been squashed.

(And it would have been questionable to squash Ard's patch with patches
authored by me.)


> Is there a way for GitHub to support both squash commits for some PRs
> and preserve a patch series for other PRs?

I don't know.

But honestly, what is the purpose of squash merges? Don't we expect
*all* contributions in well structured patch series?

If review discovers an issue with the structure of the series (for
example, not buildable in the middle, or the steps are not logical in
that order, or some patches do too many things at once and should be
split up, or a patch in the middle has a typo), the fixes shouldn't be
just heaped on top. The submitter should restructure (rebase) the
series, and submit the reworked series on a new topic branch / pull
request.

Isn't this the contribution pattern we'd like to see in all
sub-projects, where we (the TianoCore community) have reviewer /
maintainer responsibilities?

Thanks,
Laszlo

> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
>> On Behalf Of Laszlo Ersek
>> Sent: Wednesday, May 8, 2019 2:56 AM
>> To: devel@edk2.groups.io; sean.brogan@microsoft.com
>> Subject: Re: [edk2-devel] RFC for Edk2-Library
>>
>> On 05/07/19 23:35, Sean via Groups.Io wrote:
>>> RFC  Edk2-Library creation
>>>
>>> Create a new tianocore owned repository to host a
>> python library
>>> package in support of UEFI development.  This package
>> will allow easy
>>> sharing of python library code to facilitate reuse.
>> Inclusion of this
>>> package and dependency management should be managed
>> using Pip/Pypi. To
>>> start this is a supplemental package and is not
>> required to be used
>>> for edk2 builds.
>>
>> [1]
>>
>>> Examples of content here
>>>
>>> * Edk2 file type parsing
>>> * UEFI structure encode/decode in python
>>> * Packaging tools (Capsules generation, signing, INF
>> gen, Cat gen)
>>> * TPM support code
>>> * Potentially move content from
>> basetools/source/python/common/*
>>
>> [2]
>>
>>> * No command line tools/interface
>>>
>>> Maintainers
>>>
>>> * Sean Brogan
>>> * Bret Barkelew
>>> * Placeholder for existing maintainer from the
>> basetools
>>>
>>> License
>>>
>>> * BSD + Patent (edk2 aligned)
>>>
>>> Contribution process and issue tracking
>>>
>>> * Follow Github PR process for contributions and issue
>> tracking
>>> * Contributor forks repo in github
>>> * Contributor creates branch for work
>>> * Contributor updates release notes to indicate change
>> (if necessary)
>>> * Contributor submits PR to master branch of
>> tianocore/Edk2-Library
>>>   repo
>>> * Review feedback is given in PR
>>> * Python Tests are run on the repo (new contributions
>> need unit tests)
>>> * Python Style (flake8) must pass
>>> * All review feedback must be completed, maintainers
>> approved, and
>>>   tests run successfully before PR is *squash merged*
>> into master
>>
>> The sentences
>>
>> [1] "To start this is a supplemental package and is not
>> required to be
>>      used for edk2 builds."
>>
>> [2] "Potentially move content from
>> basetools/source/python/common/*"
>>
>> foreshadow that such a code movement might happen down
>> the road, and the
>> external package could become a requirement for building
>> edk2.
>>
>> That step would mean the following:
>>
>> (a) Edk2 would not remain buildable from a single
>> command
>>
>>     git clone --recurse-submodules
>>
>>     Building edk2 would require GNU/Linux users to start
>> tracking
>>     packages with "pip", which is independent of any
>> given distro's own
>>     package management, and may cause conflicts if not
>> used carefully:
>>
>>
>> https://developer.fedoraproject.org/tech/languages/pytho
>> n/pypi-installation.html
>>
>>     This requirement on "pip" would only go away once
>> the external
>>     python dependencies were packaged for at least the
>> larger GNU/Linux
>>     distros.
>>
>> (b) Edk2 users running into build problems related to
>> the external
>>     python dependencies would have to contribute through
>> a github-only
>>     workflow. That's not a deal-breaker per se -- if we
>> want to
>>     contribute to other edk2 dependencies, such as
>> OpenSSL or nasm, we
>>     also have to conform to their specific development
>> models, clearly.
>>
>>     However, "squash merge" is a catastrophically bad
>> development model,
>>     and I'd object to introducing a new edk2 build
>> dependency that
>>     followed that model.
>>
>>     (There are other issues with the GitHub.com
>> development workflow, as
>>     discussed elsewhere, but "squash merge" takes the
>> cake.)
>>
>> Problem (a) would be solvable in the long term (through
>> collaboration
>> with distro maintainers, i.e. downstream packaging), but
>> problem (b)
>> would not be. Thus I'm fine with the proposal, in its
>> current form, only
>> if the new package is 100% an addition on top of edk2,
>> even in the long
>> term, and not in any part intended as a future
>> replacement for current
>> edk2 functionality.
>>
>> Thanks,
>> Laszlo
>>
>> 
>

Re: [edk2-devel] RFC for Edk2-Library

[Michael D Kinney]

Laszlo,

1) We also use OpenSSL command line tool to locally sign
   capsules and recovery images for local testing.  So both 
   tool dependency and source dependency apply to the OpenSSL
   content.

2) If a dev submits a PR, and there are many review comments
   that require code changes, then those code changes are
   added to that PR until all feedback is addressed.  At that
   point, if the change is something that should be in a single
   commit, then doing a squash merge with a cleaned up commit
   message would be appropriate.  And the history of all the
   review feedback preserved in the PR.

   If we create a 2nd PR with the cleaned up content, then the
   connection to the 1st PR feedback may be lost.  I agree this 
   matches what we do in email reviews to create a V2 patch series.
   The V1 and V2 threads are in the email archive.  I wonder if 
   there is a way to link a 2nd PR to the 1st PR and guarantee
   that both PRs are preserved?  This would also allow the 2nd
   cleaned up PR to contain a series, and if there was a way to
   make the squash merge optional, then the developer can choose
   the way the patches are committed.

   Just a few ideas to explore...Perhaps Sean can provide some
   additional ideas to manage complex changes in PRs.

Best regards,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, May 9, 2019 3:56 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; sean.brogan@microsoft.com
> Subject: Re: [edk2-devel] RFC for Edk2-Library
> 
> On 05/09/19 20:06, Kinney, Michael D wrote:
> > Hello,
> >
> > It is difficult to tell if the repo name edk2-library
> is for firmware
> > or tools, so I recommend we work on a name that clearly
> identifies
> > that this repo is related to tools.
> 
> Good idea.
> 
> > For the pip dependencies, is the concern that a
> platform that depends
> > on these tools will not be buildable without running a
> "pip install"
> > command that pulls content from the network?
> 
> Almost. Not exactly.
> 
> First, the concern is not specific to any given platform.
> My
> understanding is that the extraction would target, in the
> longer term,
> general BaseTools functionality. That would impact all
> platforms that
> consume & build against edk2.
> 
> Second, it's not the network access that's concerning per
> se. It's the
> compatibility / interoperation with any given Linux
> distribution's own
> (native) package management system. Such a package
> management system is
> generally not used on Windows, therefore Windows users
> are accustomed to
> installing software packages from a boatload of vendors.
> This is not the
> case on Linux distros -- your distro vendor offers a
> curated set of
> packages, such that everything interoperates with
> everything (ideally),
> there are no file conflicts, version dependencies are
> handled
> automatically (e.g. if you install a high-level package,
> its
> dependencies are pulled in automatically), and so on.
> 
> Clearly the distro vendor does not *author* all this
> software (although
> they are strongly encouraged to contribute to the
> upstream software
> projects they package and ship). The distro vendor is
> responsible for
> the integration of all these packages into a consistent
> OS, into
> consistent feature sets, and so on.
> 
> "pip" and similar tools are generally unfit for this
> approach, because
> they implement a parallel package management system, to
> my
> understanding. If they are carefully used in a user's
> home directory,
> they might work. If packages were installed with "pip"
> system-wide, they
> would almost certainly break (conflict with) the distro-
> provided
> packages.
> 
> Therefore, when users would like to get a new piece of
> software
> packaged, or an existent package refreshed (to a more
> recent upstream
> version), they file a feature requst with their distro
> vendor (unless
> the distro vendor already tracks the upstream project
> closely and
> performs periodic rebases anyway). Then the distro vendor
> ships a new
> (or refreshed) package, again nicely integrated with the
> rest of the
> system.
> 
> > We already have to pull content to get the sources and
> potentially
> > other dependent tools (NASM, iASL, openssl).
> 
> Yes, these are good examples to demonstrate the
> difference. Consider
> NASM. If a Windows user would like to install NASM, they
> google "NASM
> for Windows", then go to:
> 
> https://www.nasm.us/pub/nasm/releasebuilds/2.14.02/win64/
> 
> download the ZIP file or EXE file, and install it.
> 
> In comparison, a Fedora user runs
> 
>   dnf install nasm
> 
> and they get the package from the Fedora package
> repository. A Debian
> user would run
> 
>   apt-get install nasm
> 
> and they would get the package (which would be entirely
> independent of
> Fedora's) from the Debian package repository.
> 
> The various *BSDs would run a variant of
> 
>   pkg_add nasm
> 
> I believe.
> 
> The same applies to iASL.
> 
> 
> OpenSSL is a special case. It is different from NASM and
> iASL. NASM and
> iASL are native (hosted) applications, so any Linux
> distro can easily
> build them (for itself), and the binaries can be shipped
> to users, ready
> to run. But for the purposes of edk2, OpenSSL is not
> needed as a native
> (hosted) utility, like NASM and iASL -- it is needed as a
> *cross-built*
> static library, targeting the firmware architecture (not
> the host
> architecture). Unfortunately, OpenSSL (the upstream
> project) does not
> fully support this use case yet, therefore Linux distros
> cannot just
> cross-compile OpenSSL for edk2, and offer static library
> packages. This
> is why upstream edk2 consumes OpenSSL through a git
> submodule, in source
> form.
> 
> *However*: conceptually, there is no difference. In
> Fedora and RHEL, we
> don't build OVMF against upstream OpenSSL. We build it
> against the
> downstream OpenSSL source code. So ultimately there is no
> difference in
> the distro vendor's approach -- the sources and the
> binaries come from
> the distro vendor.
> 
> 
> So what follows from this? It means that, for using
> BaseTools on a Linux
> distro, users will have to install some extra Python
> packages, in
> addition to NASM and iASL, from their vendor's
> repository. In other
> words, the "edk2-library" (or "edk2-tools") project
> should do upstream
> releases, so that Linux distros can package them in their
> native package
> systems. And some collaboration between upstream and
> downstreams is
> expected for this. (Exactly as it occurs with the iASL
> and NASM upstream
> projects.)
> 
> > Can we limit the initial scope to tools that layer on
> top of edk2, and
> > a different future RFC would be required if there is a
> proposal for
> > the edk2 repo to depend on another repo?
> 
> Yes, that would be very nice.
> 
> > If we accept this limited scope, are there still
> concerns about
> > initially using squash commits for changes to these
> tools.
> 
> Yes, I still have the same concern -- although it's a
> milder concern,
> dependent on how long squash merges are tolerated.
> 
> Assume that a second, separate RFC gets posted down the
> road, let's say
> a year in, which proposes to replace the in-line
> component FooBar of
> BaseTools, with the external project "edk2-library" (or
> "edk2-tools").
> At that point, the git history accrued thus far, in edk2-
> tools, would
> suddenly be inherited by all BaseTools users. And that
> history wouldn't
> be really helpful.
> 
> For an example, we need not look past the edk2 repository
> itself. Let's
> say I'd like to learn about the implementation history of
> the
> HandleProtocol() boot service, in edk2. So I run:
> 
> $ git blame master -- MdeModulePkg/Core/Dxe/Hand/Handle.c
> 
> and I look up the CoreHandleProtocol() function, and find
> this:
> 
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  931)
> EFI_STATUS
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  932) EFIAPI
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  933)
> CoreHandleProtocol (
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  934)   IN
> EFI_HANDLE       UserHandle,
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  935)   IN
> EFI_GUID         *Protocol,
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  936)   OUT
> VOID            **Interface
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  937)   )
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  938) {
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  939)
> return CoreOpenProtocol (
> > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> (qhuang8          2008-07-24 02:54:45 +0000  940)
> UserHandle,
> > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> (qhuang8          2008-07-24 02:54:45 +0000  941)
> Protocol,
> > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> (qhuang8          2008-07-24 02:54:45 +0000  942)
> Interface,
> > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> (qhuang8          2008-07-24 02:54:45 +0000  943)
> gDxeCoreImageHandle,
> > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> (qhuang8          2008-07-24 02:54:45 +0000  944)
> NULL,
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  945)
> EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  946)
> );
> > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> (yshang1          2007-07-04 10:51:54 +0000  947) }
> 
> Interesting, it seems like the function was originally
> added in commit
> 28a00297189c3, and then the argument list was modified in
> commit
> 022c6d45ef786, a year later. So let's check out the
> rationale for that
> argument list update:
> 
> $ git show --color 022c6d45ef786
> 
> Well... The commit message says, "Code Scrub for Dxe
> Core". That's all.
> We also learn it comes from SVN revision 5560. And the
> diffstat for the
> patch is:
> 
> >  38 files changed, 2499 insertions(+), 2504 deletions(-
> )
> 
> It is not overly helpful.
> 
> Clearly, CoreHandleProtocol() is a super-robust, super-
> mature function,
> so we don't really care for its development history
> today.
> 
> I believe the same would not apply to the "edk2-library"
> (or
> "edk2-tools") project. When it replaced the in-line
> component FooBar of
> BaseTools, it's plausible it would be suddenly exposed to
> a larger
> userbase. New issues would be encountered, and people
> might want to look
> at the git history -- if for nothing else, then rationale
> (commit
> messages) on small parts of the code.
> 
> 
> I can also name more recent examples. I'm not a frequent
> BaseTools
> contributor, but it has happened occasionally. (I've got
> 41 patches in
> the edk2 git history that touched BaseTools/, as of
> commit
> 0a506fc7ab8b.)
> 
> * For
> <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>, I
> pushed
>   5+1 patches:
> 
>     67983484a443 BaseTools/footer.makefile: expand
> BUILD_CFLAGS last for C files too
>     03252ae287c4 BaseTools/header.makefile: remove "-c"
> from BUILD_CFLAGS
>     b8a661702643 BaseTools/Source/C: split "-O2" to
> BUILD_OPTFLAGS
>     b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS
> from the caller
>     81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS
> from the caller
>     +
>     aa4e0df1f0c7 BaseTools/VfrCompile: honor
> EXTRA_LDFLAGS
> 
> * For
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>, I
> pushed 26
>   patches:
> 
>     8ff122119941 EmulatorPkg: require GCC48 or later
>     8d7cdfae8cb8 OvmfPkg: require GCC48 or later
>     fd158437dcce Vlv2TbltDevicePkg: assume GCC48 or later
>     7a9dbf2c94d1 BaseTools/Conf/tools_def.template: drop
> ARM/AARCH support from GCC46/GCC47
>     48e64498c961 BaseTools/tools_def.template: fix up LF-
> only line terminator
>     7381a6627a66 BaseTools/tools_def.template: strip
> trailing whitespace
>     9bbf156faaad BaseTools/tools_def.template: remove
> GCC48_IA32_X64_DLINK_COMMON dead-end
>     3c5613c5932c BaseTools/tools_def.template: remove
> GCC47 leaf definitions
>     fc87b8d7f411 BaseTools/tools_def.template: propagate
> loss of GCC47 references
>     91a67e0f111e BaseTools/tools_def.template: remove
> GCC47 documentation
>     0f234fb8a662 BaseTools/tools_def.template: remove
> GCC46 leaf definitions
>     83a8f313884a BaseTools/tools_def.template: propagate
> loss of GCC46 references
>     be359fa7ceec BaseTools/tools_def.template: remove
> GCC46 documentation
>     1458af0cbce0 BaseTools/tools_def.template: remove
> GCC45 leaf definitions
>     024576896d42 BaseTools/tools_def.template: propagate
> loss of GCC45 references
>     3e77d20f5cb3 BaseTools/tools_def.template: remove
> GCC45 documentation
>     e046dc60fb89 BaseTools/tools_def.template: remove
> GCC44 leaf definitions
>     38c570efede0 BaseTools/tools_def.template: propagate
> loss of GCC44 references
>     383d29096846 BaseTools/tools_def.template: rename
> GCC44_ALL_CC_FLAGS to GCC48_ALL_CC_FLAGS
>     0db91daf5228 BaseTools/tools_def.template: eliminate
> GCC44_IA32_X64_DLINK_FLAGS
>     84d21abf4e36 BaseTools/tools_def.template: rename
> GCC44_IA32_X64_DLINK_COMMON to
> GCC48_IA32_X64_DLINK_COMMON
>     5c6ccd53244b BaseTools/tools_def.template: remove
> comment about GCC44 + LzmaF86Compress
>     3bc65326d6ed BaseTools/tools_def.template: remove
> GCC44 documentation
>     f7282023e758 ArmPkg/ArmSoftFloatLib: drop build flags
> specific to GCC46/GCC47
>     300b8c5f150c CryptoPkg/BaseCryptLib: drop build flags
> specific to GCC44
>     7423ba9d499b Revert "MdePkg: avoid
> __builtin_unreachable() on GCC v4.4"
> 
>   (Note, from this list, 7a9dbf2c94d1 was authored by
> Ard, I just
>   included it at the right spot.)
> 
> All of these patch series were carefully structured, and
> carefully
> documented (in the commit messages). I would have been
> devastated, had
> they been squashed.
> 
> (And it would have been questionable to squash Ard's
> patch with patches
> authored by me.)
> 
> 
> > Is there a way for GitHub to support both squash
> commits for some PRs
> > and preserve a patch series for other PRs?
> 
> I don't know.
> 
> But honestly, what is the purpose of squash merges? Don't
> we expect
> *all* contributions in well structured patch series?
> 
> If review discovers an issue with the structure of the
> series (for
> example, not buildable in the middle, or the steps are
> not logical in
> that order, or some patches do too many things at once
> and should be
> split up, or a patch in the middle has a typo), the fixes
> shouldn't be
> just heaped on top. The submitter should restructure
> (rebase) the
> series, and submit the reworked series on a new topic
> branch / pull
> request.
> 
> Isn't this the contribution pattern we'd like to see in
> all
> sub-projects, where we (the TianoCore community) have
> reviewer /
> maintainer responsibilities?
> 
> Thanks,
> Laszlo
> 
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Wednesday, May 8, 2019 2:56 AM
> >> To: devel@edk2.groups.io; sean.brogan@microsoft.com
> >> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>
> >> On 05/07/19 23:35, Sean via Groups.Io wrote:
> >>> RFC  Edk2-Library creation
> >>>
> >>> Create a new tianocore owned repository to host a
> >> python library
> >>> package in support of UEFI development.  This package
> >> will allow easy
> >>> sharing of python library code to facilitate reuse.
> >> Inclusion of this
> >>> package and dependency management should be managed
> >> using Pip/Pypi. To
> >>> start this is a supplemental package and is not
> >> required to be used
> >>> for edk2 builds.
> >>
> >> [1]
> >>
> >>> Examples of content here
> >>>
> >>> * Edk2 file type parsing
> >>> * UEFI structure encode/decode in python
> >>> * Packaging tools (Capsules generation, signing, INF
> >> gen, Cat gen)
> >>> * TPM support code
> >>> * Potentially move content from
> >> basetools/source/python/common/*
> >>
> >> [2]
> >>
> >>> * No command line tools/interface
> >>>
> >>> Maintainers
> >>>
> >>> * Sean Brogan
> >>> * Bret Barkelew
> >>> * Placeholder for existing maintainer from the
> >> basetools
> >>>
> >>> License
> >>>
> >>> * BSD + Patent (edk2 aligned)
> >>>
> >>> Contribution process and issue tracking
> >>>
> >>> * Follow Github PR process for contributions and
> issue
> >> tracking
> >>> * Contributor forks repo in github
> >>> * Contributor creates branch for work
> >>> * Contributor updates release notes to indicate
> change
> >> (if necessary)
> >>> * Contributor submits PR to master branch of
> >> tianocore/Edk2-Library
> >>>   repo
> >>> * Review feedback is given in PR
> >>> * Python Tests are run on the repo (new contributions
> >> need unit tests)
> >>> * Python Style (flake8) must pass
> >>> * All review feedback must be completed, maintainers
> >> approved, and
> >>>   tests run successfully before PR is *squash merged*
> >> into master
> >>
> >> The sentences
> >>
> >> [1] "To start this is a supplemental package and is
> not
> >> required to be
> >>      used for edk2 builds."
> >>
> >> [2] "Potentially move content from
> >> basetools/source/python/common/*"
> >>
> >> foreshadow that such a code movement might happen down
> >> the road, and the
> >> external package could become a requirement for
> building
> >> edk2.
> >>
> >> That step would mean the following:
> >>
> >> (a) Edk2 would not remain buildable from a single
> >> command
> >>
> >>     git clone --recurse-submodules
> >>
> >>     Building edk2 would require GNU/Linux users to
> start
> >> tracking
> >>     packages with "pip", which is independent of any
> >> given distro's own
> >>     package management, and may cause conflicts if not
> >> used carefully:
> >>
> >>
> >>
> https://developer.fedoraproject.org/tech/languages/pytho
> >> n/pypi-installation.html
> >>
> >>     This requirement on "pip" would only go away once
> >> the external
> >>     python dependencies were packaged for at least the
> >> larger GNU/Linux
> >>     distros.
> >>
> >> (b) Edk2 users running into build problems related to
> >> the external
> >>     python dependencies would have to contribute
> through
> >> a github-only
> >>     workflow. That's not a deal-breaker per se -- if
> we
> >> want to
> >>     contribute to other edk2 dependencies, such as
> >> OpenSSL or nasm, we
> >>     also have to conform to their specific development
> >> models, clearly.
> >>
> >>     However, "squash merge" is a catastrophically bad
> >> development model,
> >>     and I'd object to introducing a new edk2 build
> >> dependency that
> >>     followed that model.
> >>
> >>     (There are other issues with the GitHub.com
> >> development workflow, as
> >>     discussed elsewhere, but "squash merge" takes the
> >> cake.)
> >>
> >> Problem (a) would be solvable in the long term
> (through
> >> collaboration
> >> with distro maintainers, i.e. downstream packaging),
> but
> >> problem (b)
> >> would not be. Thus I'm fine with the proposal, in its
> >> current form, only
> >> if the new package is 100% an addition on top of edk2,
> >> even in the long
> >> term, and not in any part intended as a future
> >> replacement for current
> >> edk2 functionality.
> >>
> >> Thanks,
> >> Laszlo
> >>
> >> 
> >

Re: [edk2-devel] RFC for Edk2-Library

[Michael D Kinney]

Laszlo,

I forgot to mention that pip supports virtual environments.

https://packaging.python.org/guides/installing-using-pip-and-virtual-environments/

This means that a virtual environment can be created for EDK II
builds and pip install operations can be performed in that EDK II
specific virtual environment and will not impact the system wide
pip installations.

Mike

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, May 9, 2019 5:02 PM
> To: Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; sean.brogan@microsoft.com; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] RFC for Edk2-Library
> 
> Laszlo,
> 
> 1) We also use OpenSSL command line tool to locally sign
>    capsules and recovery images for local testing.  So
> both
>    tool dependency and source dependency apply to the
> OpenSSL
>    content.
> 
> 2) If a dev submits a PR, and there are many review
> comments
>    that require code changes, then those code changes are
>    added to that PR until all feedback is addressed.  At
> that
>    point, if the change is something that should be in a
> single
>    commit, then doing a squash merge with a cleaned up
> commit
>    message would be appropriate.  And the history of all
> the
>    review feedback preserved in the PR.
> 
>    If we create a 2nd PR with the cleaned up content,
> then the
>    connection to the 1st PR feedback may be lost.  I
> agree this
>    matches what we do in email reviews to create a V2
> patch series.
>    The V1 and V2 threads are in the email archive.  I
> wonder if
>    there is a way to link a 2nd PR to the 1st PR and
> guarantee
>    that both PRs are preserved?  This would also allow
> the 2nd
>    cleaned up PR to contain a series, and if there was a
> way to
>    make the squash merge optional, then the developer can
> choose
>    the way the patches are committed.
> 
>    Just a few ideas to explore...Perhaps Sean can provide
> some
>    additional ideas to manage complex changes in PRs.
> 
> Best regards,
> 
> Mike
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Thursday, May 9, 2019 3:56 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io; sean.brogan@microsoft.com
> > Subject: Re: [edk2-devel] RFC for Edk2-Library
> >
> > On 05/09/19 20:06, Kinney, Michael D wrote:
> > > Hello,
> > >
> > > It is difficult to tell if the repo name edk2-library
> > is for firmware
> > > or tools, so I recommend we work on a name that
> clearly
> > identifies
> > > that this repo is related to tools.
> >
> > Good idea.
> >
> > > For the pip dependencies, is the concern that a
> > platform that depends
> > > on these tools will not be buildable without running
> a
> > "pip install"
> > > command that pulls content from the network?
> >
> > Almost. Not exactly.
> >
> > First, the concern is not specific to any given
> platform.
> > My
> > understanding is that the extraction would target, in
> the
> > longer term,
> > general BaseTools functionality. That would impact all
> > platforms that
> > consume & build against edk2.
> >
> > Second, it's not the network access that's concerning
> per
> > se. It's the
> > compatibility / interoperation with any given Linux
> > distribution's own
> > (native) package management system. Such a package
> > management system is
> > generally not used on Windows, therefore Windows users
> > are accustomed to
> > installing software packages from a boatload of
> vendors.
> > This is not the
> > case on Linux distros -- your distro vendor offers a
> > curated set of
> > packages, such that everything interoperates with
> > everything (ideally),
> > there are no file conflicts, version dependencies are
> > handled
> > automatically (e.g. if you install a high-level
> package,
> > its
> > dependencies are pulled in automatically), and so on.
> >
> > Clearly the distro vendor does not *author* all this
> > software (although
> > they are strongly encouraged to contribute to the
> > upstream software
> > projects they package and ship). The distro vendor is
> > responsible for
> > the integration of all these packages into a consistent
> > OS, into
> > consistent feature sets, and so on.
> >
> > "pip" and similar tools are generally unfit for this
> > approach, because
> > they implement a parallel package management system, to
> > my
> > understanding. If they are carefully used in a user's
> > home directory,
> > they might work. If packages were installed with "pip"
> > system-wide, they
> > would almost certainly break (conflict with) the
> distro-
> > provided
> > packages.
> >
> > Therefore, when users would like to get a new piece of
> > software
> > packaged, or an existent package refreshed (to a more
> > recent upstream
> > version), they file a feature requst with their distro
> > vendor (unless
> > the distro vendor already tracks the upstream project
> > closely and
> > performs periodic rebases anyway). Then the distro
> vendor
> > ships a new
> > (or refreshed) package, again nicely integrated with
> the
> > rest of the
> > system.
> >
> > > We already have to pull content to get the sources
> and
> > potentially
> > > other dependent tools (NASM, iASL, openssl).
> >
> > Yes, these are good examples to demonstrate the
> > difference. Consider
> > NASM. If a Windows user would like to install NASM,
> they
> > google "NASM
> > for Windows", then go to:
> >
> >
> https://www.nasm.us/pub/nasm/releasebuilds/2.14.02/win64/
> >
> > download the ZIP file or EXE file, and install it.
> >
> > In comparison, a Fedora user runs
> >
> >   dnf install nasm
> >
> > and they get the package from the Fedora package
> > repository. A Debian
> > user would run
> >
> >   apt-get install nasm
> >
> > and they would get the package (which would be entirely
> > independent of
> > Fedora's) from the Debian package repository.
> >
> > The various *BSDs would run a variant of
> >
> >   pkg_add nasm
> >
> > I believe.
> >
> > The same applies to iASL.
> >
> >
> > OpenSSL is a special case. It is different from NASM
> and
> > iASL. NASM and
> > iASL are native (hosted) applications, so any Linux
> > distro can easily
> > build them (for itself), and the binaries can be
> shipped
> > to users, ready
> > to run. But for the purposes of edk2, OpenSSL is not
> > needed as a native
> > (hosted) utility, like NASM and iASL -- it is needed as
> a
> > *cross-built*
> > static library, targeting the firmware architecture
> (not
> > the host
> > architecture). Unfortunately, OpenSSL (the upstream
> > project) does not
> > fully support this use case yet, therefore Linux
> distros
> > cannot just
> > cross-compile OpenSSL for edk2, and offer static
> library
> > packages. This
> > is why upstream edk2 consumes OpenSSL through a git
> > submodule, in source
> > form.
> >
> > *However*: conceptually, there is no difference. In
> > Fedora and RHEL, we
> > don't build OVMF against upstream OpenSSL. We build it
> > against the
> > downstream OpenSSL source code. So ultimately there is
> no
> > difference in
> > the distro vendor's approach -- the sources and the
> > binaries come from
> > the distro vendor.
> >
> >
> > So what follows from this? It means that, for using
> > BaseTools on a Linux
> > distro, users will have to install some extra Python
> > packages, in
> > addition to NASM and iASL, from their vendor's
> > repository. In other
> > words, the "edk2-library" (or "edk2-tools") project
> > should do upstream
> > releases, so that Linux distros can package them in
> their
> > native package
> > systems. And some collaboration between upstream and
> > downstreams is
> > expected for this. (Exactly as it occurs with the iASL
> > and NASM upstream
> > projects.)
> >
> > > Can we limit the initial scope to tools that layer on
> > top of edk2, and
> > > a different future RFC would be required if there is
> a
> > proposal for
> > > the edk2 repo to depend on another repo?
> >
> > Yes, that would be very nice.
> >
> > > If we accept this limited scope, are there still
> > concerns about
> > > initially using squash commits for changes to these
> > tools.
> >
> > Yes, I still have the same concern -- although it's a
> > milder concern,
> > dependent on how long squash merges are tolerated.
> >
> > Assume that a second, separate RFC gets posted down the
> > road, let's say
> > a year in, which proposes to replace the in-line
> > component FooBar of
> > BaseTools, with the external project "edk2-library" (or
> > "edk2-tools").
> > At that point, the git history accrued thus far, in
> edk2-
> > tools, would
> > suddenly be inherited by all BaseTools users. And that
> > history wouldn't
> > be really helpful.
> >
> > For an example, we need not look past the edk2
> repository
> > itself. Let's
> > say I'd like to learn about the implementation history
> of
> > the
> > HandleProtocol() boot service, in edk2. So I run:
> >
> > $ git blame master --
> MdeModulePkg/Core/Dxe/Hand/Handle.c
> >
> > and I look up the CoreHandleProtocol() function, and
> find
> > this:
> >
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  931)
> > EFI_STATUS
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  932)
> EFIAPI
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  933)
> > CoreHandleProtocol (
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  934)   IN
> > EFI_HANDLE       UserHandle,
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  935)   IN
> > EFI_GUID         *Protocol,
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  936)   OUT
> > VOID            **Interface
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  937)   )
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  938) {
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  939)
> > return CoreOpenProtocol (
> > > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> > (qhuang8          2008-07-24 02:54:45 +0000  940)
> > UserHandle,
> > > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> > (qhuang8          2008-07-24 02:54:45 +0000  941)
> > Protocol,
> > > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> > (qhuang8          2008-07-24 02:54:45 +0000  942)
> > Interface,
> > > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> > (qhuang8          2008-07-24 02:54:45 +0000  943)
> > gDxeCoreImageHandle,
> > > 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> > (qhuang8          2008-07-24 02:54:45 +0000  944)
> > NULL,
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  945)
> > EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  946)
> > );
> > > 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> > (yshang1          2007-07-04 10:51:54 +0000  947) }
> >
> > Interesting, it seems like the function was originally
> > added in commit
> > 28a00297189c3, and then the argument list was modified
> in
> > commit
> > 022c6d45ef786, a year later. So let's check out the
> > rationale for that
> > argument list update:
> >
> > $ git show --color 022c6d45ef786
> >
> > Well... The commit message says, "Code Scrub for Dxe
> > Core". That's all.
> > We also learn it comes from SVN revision 5560. And the
> > diffstat for the
> > patch is:
> >
> > >  38 files changed, 2499 insertions(+), 2504
> deletions(-
> > )
> >
> > It is not overly helpful.
> >
> > Clearly, CoreHandleProtocol() is a super-robust, super-
> > mature function,
> > so we don't really care for its development history
> > today.
> >
> > I believe the same would not apply to the "edk2-
> library"
> > (or
> > "edk2-tools") project. When it replaced the in-line
> > component FooBar of
> > BaseTools, it's plausible it would be suddenly exposed
> to
> > a larger
> > userbase. New issues would be encountered, and people
> > might want to look
> > at the git history -- if for nothing else, then
> rationale
> > (commit
> > messages) on small parts of the code.
> >
> >
> > I can also name more recent examples. I'm not a
> frequent
> > BaseTools
> > contributor, but it has happened occasionally. (I've
> got
> > 41 patches in
> > the edk2 git history that touched BaseTools/, as of
> > commit
> > 0a506fc7ab8b.)
> >
> > * For
> > <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>,
> I
> > pushed
> >   5+1 patches:
> >
> >     67983484a443 BaseTools/footer.makefile: expand
> > BUILD_CFLAGS last for C files too
> >     03252ae287c4 BaseTools/header.makefile: remove "-c"
> > from BUILD_CFLAGS
> >     b8a661702643 BaseTools/Source/C: split "-O2" to
> > BUILD_OPTFLAGS
> >     b0ca5dae78ff BaseTools/Source/C: take
> EXTRA_OPTFLAGS
> > from the caller
> >     81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS
> > from the caller
> >     +
> >     aa4e0df1f0c7 BaseTools/VfrCompile: honor
> > EXTRA_LDFLAGS
> >
> > * For
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>,
> I
> > pushed 26
> >   patches:
> >
> >     8ff122119941 EmulatorPkg: require GCC48 or later
> >     8d7cdfae8cb8 OvmfPkg: require GCC48 or later
> >     fd158437dcce Vlv2TbltDevicePkg: assume GCC48 or
> later
> >     7a9dbf2c94d1 BaseTools/Conf/tools_def.template:
> drop
> > ARM/AARCH support from GCC46/GCC47
> >     48e64498c961 BaseTools/tools_def.template: fix up
> LF-
> > only line terminator
> >     7381a6627a66 BaseTools/tools_def.template: strip
> > trailing whitespace
> >     9bbf156faaad BaseTools/tools_def.template: remove
> > GCC48_IA32_X64_DLINK_COMMON dead-end
> >     3c5613c5932c BaseTools/tools_def.template: remove
> > GCC47 leaf definitions
> >     fc87b8d7f411 BaseTools/tools_def.template:
> propagate
> > loss of GCC47 references
> >     91a67e0f111e BaseTools/tools_def.template: remove
> > GCC47 documentation
> >     0f234fb8a662 BaseTools/tools_def.template: remove
> > GCC46 leaf definitions
> >     83a8f313884a BaseTools/tools_def.template:
> propagate
> > loss of GCC46 references
> >     be359fa7ceec BaseTools/tools_def.template: remove
> > GCC46 documentation
> >     1458af0cbce0 BaseTools/tools_def.template: remove
> > GCC45 leaf definitions
> >     024576896d42 BaseTools/tools_def.template:
> propagate
> > loss of GCC45 references
> >     3e77d20f5cb3 BaseTools/tools_def.template: remove
> > GCC45 documentation
> >     e046dc60fb89 BaseTools/tools_def.template: remove
> > GCC44 leaf definitions
> >     38c570efede0 BaseTools/tools_def.template:
> propagate
> > loss of GCC44 references
> >     383d29096846 BaseTools/tools_def.template: rename
> > GCC44_ALL_CC_FLAGS to GCC48_ALL_CC_FLAGS
> >     0db91daf5228 BaseTools/tools_def.template:
> eliminate
> > GCC44_IA32_X64_DLINK_FLAGS
> >     84d21abf4e36 BaseTools/tools_def.template: rename
> > GCC44_IA32_X64_DLINK_COMMON to
> > GCC48_IA32_X64_DLINK_COMMON
> >     5c6ccd53244b BaseTools/tools_def.template: remove
> > comment about GCC44 + LzmaF86Compress
> >     3bc65326d6ed BaseTools/tools_def.template: remove
> > GCC44 documentation
> >     f7282023e758 ArmPkg/ArmSoftFloatLib: drop build
> flags
> > specific to GCC46/GCC47
> >     300b8c5f150c CryptoPkg/BaseCryptLib: drop build
> flags
> > specific to GCC44
> >     7423ba9d499b Revert "MdePkg: avoid
> > __builtin_unreachable() on GCC v4.4"
> >
> >   (Note, from this list, 7a9dbf2c94d1 was authored by
> > Ard, I just
> >   included it at the right spot.)
> >
> > All of these patch series were carefully structured,
> and
> > carefully
> > documented (in the commit messages). I would have been
> > devastated, had
> > they been squashed.
> >
> > (And it would have been questionable to squash Ard's
> > patch with patches
> > authored by me.)
> >
> >
> > > Is there a way for GitHub to support both squash
> > commits for some PRs
> > > and preserve a patch series for other PRs?
> >
> > I don't know.
> >
> > But honestly, what is the purpose of squash merges?
> Don't
> > we expect
> > *all* contributions in well structured patch series?
> >
> > If review discovers an issue with the structure of the
> > series (for
> > example, not buildable in the middle, or the steps are
> > not logical in
> > that order, or some patches do too many things at once
> > and should be
> > split up, or a patch in the middle has a typo), the
> fixes
> > shouldn't be
> > just heaped on top. The submitter should restructure
> > (rebase) the
> > series, and submit the reworked series on a new topic
> > branch / pull
> > request.
> >
> > Isn't this the contribution pattern we'd like to see in
> > all
> > sub-projects, where we (the TianoCore community) have
> > reviewer /
> > maintainer responsibilities?
> >
> > Thanks,
> > Laszlo
> >
> > > Thanks,
> > >
> > > Mike
> > >
> > >> -----Original Message-----
> > >> From: devel@edk2.groups.io
> > [mailto:devel@edk2.groups.io]
> > >> On Behalf Of Laszlo Ersek
> > >> Sent: Wednesday, May 8, 2019 2:56 AM
> > >> To: devel@edk2.groups.io; sean.brogan@microsoft.com
> > >> Subject: Re: [edk2-devel] RFC for Edk2-Library
> > >>
> > >> On 05/07/19 23:35, Sean via Groups.Io wrote:
> > >>> RFC  Edk2-Library creation
> > >>>
> > >>> Create a new tianocore owned repository to host a
> > >> python library
> > >>> package in support of UEFI development.  This
> package
> > >> will allow easy
> > >>> sharing of python library code to facilitate reuse.
> > >> Inclusion of this
> > >>> package and dependency management should be managed
> > >> using Pip/Pypi. To
> > >>> start this is a supplemental package and is not
> > >> required to be used
> > >>> for edk2 builds.
> > >>
> > >> [1]
> > >>
> > >>> Examples of content here
> > >>>
> > >>> * Edk2 file type parsing
> > >>> * UEFI structure encode/decode in python
> > >>> * Packaging tools (Capsules generation, signing,
> INF
> > >> gen, Cat gen)
> > >>> * TPM support code
> > >>> * Potentially move content from
> > >> basetools/source/python/common/*
> > >>
> > >> [2]
> > >>
> > >>> * No command line tools/interface
> > >>>
> > >>> Maintainers
> > >>>
> > >>> * Sean Brogan
> > >>> * Bret Barkelew
> > >>> * Placeholder for existing maintainer from the
> > >> basetools
> > >>>
> > >>> License
> > >>>
> > >>> * BSD + Patent (edk2 aligned)
> > >>>
> > >>> Contribution process and issue tracking
> > >>>
> > >>> * Follow Github PR process for contributions and
> > issue
> > >> tracking
> > >>> * Contributor forks repo in github
> > >>> * Contributor creates branch for work
> > >>> * Contributor updates release notes to indicate
> > change
> > >> (if necessary)
> > >>> * Contributor submits PR to master branch of
> > >> tianocore/Edk2-Library
> > >>>   repo
> > >>> * Review feedback is given in PR
> > >>> * Python Tests are run on the repo (new
> contributions
> > >> need unit tests)
> > >>> * Python Style (flake8) must pass
> > >>> * All review feedback must be completed,
> maintainers
> > >> approved, and
> > >>>   tests run successfully before PR is *squash
> merged*
> > >> into master
> > >>
> > >> The sentences
> > >>
> > >> [1] "To start this is a supplemental package and is
> > not
> > >> required to be
> > >>      used for edk2 builds."
> > >>
> > >> [2] "Potentially move content from
> > >> basetools/source/python/common/*"
> > >>
> > >> foreshadow that such a code movement might happen
> down
> > >> the road, and the
> > >> external package could become a requirement for
> > building
> > >> edk2.
> > >>
> > >> That step would mean the following:
> > >>
> > >> (a) Edk2 would not remain buildable from a single
> > >> command
> > >>
> > >>     git clone --recurse-submodules
> > >>
> > >>     Building edk2 would require GNU/Linux users to
> > start
> > >> tracking
> > >>     packages with "pip", which is independent of any
> > >> given distro's own
> > >>     package management, and may cause conflicts if
> not
> > >> used carefully:
> > >>
> > >>
> > >>
> >
> https://developer.fedoraproject.org/tech/languages/pytho
> > >> n/pypi-installation.html
> > >>
> > >>     This requirement on "pip" would only go away
> once
> > >> the external
> > >>     python dependencies were packaged for at least
> the
> > >> larger GNU/Linux
> > >>     distros.
> > >>
> > >> (b) Edk2 users running into build problems related
> to
> > >> the external
> > >>     python dependencies would have to contribute
> > through
> > >> a github-only
> > >>     workflow. That's not a deal-breaker per se -- if
> > we
> > >> want to
> > >>     contribute to other edk2 dependencies, such as
> > >> OpenSSL or nasm, we
> > >>     also have to conform to their specific
> development
> > >> models, clearly.
> > >>
> > >>     However, "squash merge" is a catastrophically
> bad
> > >> development model,
> > >>     and I'd object to introducing a new edk2 build
> > >> dependency that
> > >>     followed that model.
> > >>
> > >>     (There are other issues with the GitHub.com
> > >> development workflow, as
> > >>     discussed elsewhere, but "squash merge" takes
> the
> > >> cake.)
> > >>
> > >> Problem (a) would be solvable in the long term
> > (through
> > >> collaboration
> > >> with distro maintainers, i.e. downstream packaging),
> > but
> > >> problem (b)
> > >> would not be. Thus I'm fine with the proposal, in
> its
> > >> current form, only
> > >> if the new package is 100% an addition on top of
> edk2,
> > >> even in the long
> > >> term, and not in any part intended as a future
> > >> replacement for current
> > >> edk2 functionality.
> > >>
> > >> Thanks,
> > >> Laszlo
> > >>
> > >> 
> > >

Re: [edk2-devel] RFC for Edk2-Library

[Sean]

[-- Attachment #1: Type: text/plain, Size: 2810 bytes --]

1. Agree on the name but not sure whats better.  i don't think it should be edk2-tools because the idea of this is to be a library of support code but not the tools themselves.  This limits dependencies and keeps the library free of business specific logic.  The second RFC will be for a new repo that is more like edk2-tools.  So maybe this could be edk2-tools-library?

2. Future direction (edk2 having a dependency on this package).  I don't think this library will achieve my goals if it doesn't start to pull in support libraries from basetools.  The simplest example is the parsers.  It would seem foolish to have two copies.  It would be great to have one set of parsers that could be used by tool developers as well as the edk2 build process.  Opening up those tools would make writing edk2 analysis tools much easier/faster/better.

3. Pip/distribution/versioning.  I would definitely not want my version of this pip module dependent on my OS distro and version.  This will be something tied to the edk2 platform i am building.   These are things that are somewhat stable but i would expect more churn than an OS and different platforms will have different needs.  i also don't want to sign up for working with OS vendors to get this into their package management.  With python 3.5 and newer there is a built in concept of virtual environments (venv).  This is how i would handle this problem.  The Pip modules and their dependencies do not impact the global packages.  Only the virtual environment gets updated and a virtual environment can be trivially created/deleted.  This is also expected if you are building platforms that might be running different versions of edk2 so its a good idea to create a venv for each code tree on your system and activate that venv when doing builds in that code tree.

4. Squash merge/PR process.  We discussed this at length at plugfest and unfortunately i don't think there was any silver bullet.  Patchsets and the edk2 process today are a relic of emailing patches.  When you move to pull requests managed by a server with policy and automation i think the only feasible solution is squash merge.  Now i don't understand why squash merge means bad commit messages and lost info.  The idea is that a single PR should be a single feature.  When it gets squashed the commit message should reflect the feature and be of high quality.  If the PR was too big and contained numerous features then the PR should be split up.  Squash merge allows your automation to easily guarantee bisect-ability, build-ability, and that each commit will pass the requirements.    Having one continuous PR optimizes your review process/comment tracking, etc.  I would be strongly opposed to opening new PRs for every new "version".

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 3014 bytes --]

Re: [edk2-devel] RFC for Edk2-Library

[Laszlo Ersek]

On 05/10/19 02:01, Kinney, Michael D wrote:
> Laszlo,
> 
> 1) We also use OpenSSL command line tool to locally sign
>    capsules and recovery images for local testing.  So both 
>    tool dependency and source dependency apply to the OpenSSL
>    content.

I haven't used the tools yet that you refer to, so I'm unsure how
exactly they invoke the "openssl" utility. If they just rely on the PATH
environment variable, then what they invoke comes from the system-wide
openssl package.

> 
> 2) If a dev submits a PR, and there are many review comments
>    that require code changes, then those code changes are
>    added to that PR until all feedback is addressed.

I don't understand how. Let's say the pull request refers to a branch
with three commits, and the majority of the review comments request
updates for patch #2 (i.e., in the middle). How can the submitter "add
changes to the PR"? It is patch #2 that needs to be reworked, which will
require rebases, and so the hash of the HEAD commit of the topic branch
(patch #3) will change as well.

>    At that
>    point, if the change is something that should be in a single
>    commit, then doing a squash merge with a cleaned up commit
>    message would be appropriate.

I don't understand -- we modify only patch #2, yes, to address review
comments, but why does that justify squashing #1 through #3 into a
single commit?

>    And the history of all the
>    review feedback preserved in the PR.

That's good (but it could be better -- see the lacking email
integration. Anyway this is not strictly tied to my concern with
squash-on-merge).

> 
>    If we create a 2nd PR with the cleaned up content, then the
>    connection to the 1st PR feedback may be lost.  I agree this 
>    matches what we do in email reviews to create a V2 patch series.
>    The V1 and V2 threads are in the email archive.

Indeed. And the blurb for both threads reference the bugzilla, and the
bugzilla has (if the submitter is careful) comments linking the v1 and
v2 threads from the email archive.

>    I wonder if
>    there is a way to link a 2nd PR to the 1st PR and guarantee
>    that both PRs are preserved?  This would also allow the 2nd
>    cleaned up PR to contain a series, and if there was a way to
>    make the squash merge optional, then the developer can choose
>    the way the patches are committed.

This sounds great! I think both PRs can reference the bugzilla ticket,
and the bugzilla ticket can reference both PRs too (as comments). The
first PR can be rejected & closed when the second one is submitted.

>    Just a few ideas to explore...Perhaps Sean can provide some
>    additional ideas to manage complex changes in PRs.

Thanks!
Laszlo

> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, May 9, 2019 3:56 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> devel@edk2.groups.io; sean.brogan@microsoft.com
>> Subject: Re: [edk2-devel] RFC for Edk2-Library
>>
>> On 05/09/19 20:06, Kinney, Michael D wrote:
>>> Hello,
>>>
>>> It is difficult to tell if the repo name edk2-library
>> is for firmware
>>> or tools, so I recommend we work on a name that clearly
>> identifies
>>> that this repo is related to tools.
>>
>> Good idea.
>>
>>> For the pip dependencies, is the concern that a
>> platform that depends
>>> on these tools will not be buildable without running a
>> "pip install"
>>> command that pulls content from the network?
>>
>> Almost. Not exactly.
>>
>> First, the concern is not specific to any given platform.
>> My
>> understanding is that the extraction would target, in the
>> longer term,
>> general BaseTools functionality. That would impact all
>> platforms that
>> consume & build against edk2.
>>
>> Second, it's not the network access that's concerning per
>> se. It's the
>> compatibility / interoperation with any given Linux
>> distribution's own
>> (native) package management system. Such a package
>> management system is
>> generally not used on Windows, therefore Windows users
>> are accustomed to
>> installing software packages from a boatload of vendors.
>> This is not the
>> case on Linux distros -- your distro vendor offers a
>> curated set of
>> packages, such that everything interoperates with
>> everything (ideally),
>> there are no file conflicts, version dependencies are
>> handled
>> automatically (e.g. if you install a high-level package,
>> its
>> dependencies are pulled in automatically), and so on.
>>
>> Clearly the distro vendor does not *author* all this
>> software (although
>> they are strongly encouraged to contribute to the
>> upstream software
>> projects they package and ship). The distro vendor is
>> responsible for
>> the integration of all these packages into a consistent
>> OS, into
>> consistent feature sets, and so on.
>>
>> "pip" and similar tools are generally unfit for this
>> approach, because
>> they implement a parallel package management system, to
>> my
>> understanding. If they are carefully used in a user's
>> home directory,
>> they might work. If packages were installed with "pip"
>> system-wide, they
>> would almost certainly break (conflict with) the distro-
>> provided
>> packages.
>>
>> Therefore, when users would like to get a new piece of
>> software
>> packaged, or an existent package refreshed (to a more
>> recent upstream
>> version), they file a feature requst with their distro
>> vendor (unless
>> the distro vendor already tracks the upstream project
>> closely and
>> performs periodic rebases anyway). Then the distro vendor
>> ships a new
>> (or refreshed) package, again nicely integrated with the
>> rest of the
>> system.
>>
>>> We already have to pull content to get the sources and
>> potentially
>>> other dependent tools (NASM, iASL, openssl).
>>
>> Yes, these are good examples to demonstrate the
>> difference. Consider
>> NASM. If a Windows user would like to install NASM, they
>> google "NASM
>> for Windows", then go to:
>>
>> https://www.nasm.us/pub/nasm/releasebuilds/2.14.02/win64/
>>
>> download the ZIP file or EXE file, and install it.
>>
>> In comparison, a Fedora user runs
>>
>>   dnf install nasm
>>
>> and they get the package from the Fedora package
>> repository. A Debian
>> user would run
>>
>>   apt-get install nasm
>>
>> and they would get the package (which would be entirely
>> independent of
>> Fedora's) from the Debian package repository.
>>
>> The various *BSDs would run a variant of
>>
>>   pkg_add nasm
>>
>> I believe.
>>
>> The same applies to iASL.
>>
>>
>> OpenSSL is a special case. It is different from NASM and
>> iASL. NASM and
>> iASL are native (hosted) applications, so any Linux
>> distro can easily
>> build them (for itself), and the binaries can be shipped
>> to users, ready
>> to run. But for the purposes of edk2, OpenSSL is not
>> needed as a native
>> (hosted) utility, like NASM and iASL -- it is needed as a
>> *cross-built*
>> static library, targeting the firmware architecture (not
>> the host
>> architecture). Unfortunately, OpenSSL (the upstream
>> project) does not
>> fully support this use case yet, therefore Linux distros
>> cannot just
>> cross-compile OpenSSL for edk2, and offer static library
>> packages. This
>> is why upstream edk2 consumes OpenSSL through a git
>> submodule, in source
>> form.
>>
>> *However*: conceptually, there is no difference. In
>> Fedora and RHEL, we
>> don't build OVMF against upstream OpenSSL. We build it
>> against the
>> downstream OpenSSL source code. So ultimately there is no
>> difference in
>> the distro vendor's approach -- the sources and the
>> binaries come from
>> the distro vendor.
>>
>>
>> So what follows from this? It means that, for using
>> BaseTools on a Linux
>> distro, users will have to install some extra Python
>> packages, in
>> addition to NASM and iASL, from their vendor's
>> repository. In other
>> words, the "edk2-library" (or "edk2-tools") project
>> should do upstream
>> releases, so that Linux distros can package them in their
>> native package
>> systems. And some collaboration between upstream and
>> downstreams is
>> expected for this. (Exactly as it occurs with the iASL
>> and NASM upstream
>> projects.)
>>
>>> Can we limit the initial scope to tools that layer on
>> top of edk2, and
>>> a different future RFC would be required if there is a
>> proposal for
>>> the edk2 repo to depend on another repo?
>>
>> Yes, that would be very nice.
>>
>>> If we accept this limited scope, are there still
>> concerns about
>>> initially using squash commits for changes to these
>> tools.
>>
>> Yes, I still have the same concern -- although it's a
>> milder concern,
>> dependent on how long squash merges are tolerated.
>>
>> Assume that a second, separate RFC gets posted down the
>> road, let's say
>> a year in, which proposes to replace the in-line
>> component FooBar of
>> BaseTools, with the external project "edk2-library" (or
>> "edk2-tools").
>> At that point, the git history accrued thus far, in edk2-
>> tools, would
>> suddenly be inherited by all BaseTools users. And that
>> history wouldn't
>> be really helpful.
>>
>> For an example, we need not look past the edk2 repository
>> itself. Let's
>> say I'd like to learn about the implementation history of
>> the
>> HandleProtocol() boot service, in edk2. So I run:
>>
>> $ git blame master -- MdeModulePkg/Core/Dxe/Hand/Handle.c
>>
>> and I look up the CoreHandleProtocol() function, and find
>> this:
>>
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  931)
>> EFI_STATUS
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  932) EFIAPI
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  933)
>> CoreHandleProtocol (
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  934)   IN
>> EFI_HANDLE       UserHandle,
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  935)   IN
>> EFI_GUID         *Protocol,
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  936)   OUT
>> VOID            **Interface
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  937)   )
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  938) {
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  939)
>> return CoreOpenProtocol (
>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>> (qhuang8          2008-07-24 02:54:45 +0000  940)
>> UserHandle,
>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>> (qhuang8          2008-07-24 02:54:45 +0000  941)
>> Protocol,
>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>> (qhuang8          2008-07-24 02:54:45 +0000  942)
>> Interface,
>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>> (qhuang8          2008-07-24 02:54:45 +0000  943)
>> gDxeCoreImageHandle,
>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>> (qhuang8          2008-07-24 02:54:45 +0000  944)
>> NULL,
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  945)
>> EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  946)
>> );
>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>> (yshang1          2007-07-04 10:51:54 +0000  947) }
>>
>> Interesting, it seems like the function was originally
>> added in commit
>> 28a00297189c3, and then the argument list was modified in
>> commit
>> 022c6d45ef786, a year later. So let's check out the
>> rationale for that
>> argument list update:
>>
>> $ git show --color 022c6d45ef786
>>
>> Well... The commit message says, "Code Scrub for Dxe
>> Core". That's all.
>> We also learn it comes from SVN revision 5560. And the
>> diffstat for the
>> patch is:
>>
>>>  38 files changed, 2499 insertions(+), 2504 deletions(-
>> )
>>
>> It is not overly helpful.
>>
>> Clearly, CoreHandleProtocol() is a super-robust, super-
>> mature function,
>> so we don't really care for its development history
>> today.
>>
>> I believe the same would not apply to the "edk2-library"
>> (or
>> "edk2-tools") project. When it replaced the in-line
>> component FooBar of
>> BaseTools, it's plausible it would be suddenly exposed to
>> a larger
>> userbase. New issues would be encountered, and people
>> might want to look
>> at the git history -- if for nothing else, then rationale
>> (commit
>> messages) on small parts of the code.
>>
>>
>> I can also name more recent examples. I'm not a frequent
>> BaseTools
>> contributor, but it has happened occasionally. (I've got
>> 41 patches in
>> the edk2 git history that touched BaseTools/, as of
>> commit
>> 0a506fc7ab8b.)
>>
>> * For
>> <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>, I
>> pushed
>>   5+1 patches:
>>
>>     67983484a443 BaseTools/footer.makefile: expand
>> BUILD_CFLAGS last for C files too
>>     03252ae287c4 BaseTools/header.makefile: remove "-c"
>> from BUILD_CFLAGS
>>     b8a661702643 BaseTools/Source/C: split "-O2" to
>> BUILD_OPTFLAGS
>>     b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS
>> from the caller
>>     81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS
>> from the caller
>>     +
>>     aa4e0df1f0c7 BaseTools/VfrCompile: honor
>> EXTRA_LDFLAGS
>>
>> * For
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>, I
>> pushed 26
>>   patches:
>>
>>     8ff122119941 EmulatorPkg: require GCC48 or later
>>     8d7cdfae8cb8 OvmfPkg: require GCC48 or later
>>     fd158437dcce Vlv2TbltDevicePkg: assume GCC48 or later
>>     7a9dbf2c94d1 BaseTools/Conf/tools_def.template: drop
>> ARM/AARCH support from GCC46/GCC47
>>     48e64498c961 BaseTools/tools_def.template: fix up LF-
>> only line terminator
>>     7381a6627a66 BaseTools/tools_def.template: strip
>> trailing whitespace
>>     9bbf156faaad BaseTools/tools_def.template: remove
>> GCC48_IA32_X64_DLINK_COMMON dead-end
>>     3c5613c5932c BaseTools/tools_def.template: remove
>> GCC47 leaf definitions
>>     fc87b8d7f411 BaseTools/tools_def.template: propagate
>> loss of GCC47 references
>>     91a67e0f111e BaseTools/tools_def.template: remove
>> GCC47 documentation
>>     0f234fb8a662 BaseTools/tools_def.template: remove
>> GCC46 leaf definitions
>>     83a8f313884a BaseTools/tools_def.template: propagate
>> loss of GCC46 references
>>     be359fa7ceec BaseTools/tools_def.template: remove
>> GCC46 documentation
>>     1458af0cbce0 BaseTools/tools_def.template: remove
>> GCC45 leaf definitions
>>     024576896d42 BaseTools/tools_def.template: propagate
>> loss of GCC45 references
>>     3e77d20f5cb3 BaseTools/tools_def.template: remove
>> GCC45 documentation
>>     e046dc60fb89 BaseTools/tools_def.template: remove
>> GCC44 leaf definitions
>>     38c570efede0 BaseTools/tools_def.template: propagate
>> loss of GCC44 references
>>     383d29096846 BaseTools/tools_def.template: rename
>> GCC44_ALL_CC_FLAGS to GCC48_ALL_CC_FLAGS
>>     0db91daf5228 BaseTools/tools_def.template: eliminate
>> GCC44_IA32_X64_DLINK_FLAGS
>>     84d21abf4e36 BaseTools/tools_def.template: rename
>> GCC44_IA32_X64_DLINK_COMMON to
>> GCC48_IA32_X64_DLINK_COMMON
>>     5c6ccd53244b BaseTools/tools_def.template: remove
>> comment about GCC44 + LzmaF86Compress
>>     3bc65326d6ed BaseTools/tools_def.template: remove
>> GCC44 documentation
>>     f7282023e758 ArmPkg/ArmSoftFloatLib: drop build flags
>> specific to GCC46/GCC47
>>     300b8c5f150c CryptoPkg/BaseCryptLib: drop build flags
>> specific to GCC44
>>     7423ba9d499b Revert "MdePkg: avoid
>> __builtin_unreachable() on GCC v4.4"
>>
>>   (Note, from this list, 7a9dbf2c94d1 was authored by
>> Ard, I just
>>   included it at the right spot.)
>>
>> All of these patch series were carefully structured, and
>> carefully
>> documented (in the commit messages). I would have been
>> devastated, had
>> they been squashed.
>>
>> (And it would have been questionable to squash Ard's
>> patch with patches
>> authored by me.)
>>
>>
>>> Is there a way for GitHub to support both squash
>> commits for some PRs
>>> and preserve a patch series for other PRs?
>>
>> I don't know.
>>
>> But honestly, what is the purpose of squash merges? Don't
>> we expect
>> *all* contributions in well structured patch series?
>>
>> If review discovers an issue with the structure of the
>> series (for
>> example, not buildable in the middle, or the steps are
>> not logical in
>> that order, or some patches do too many things at once
>> and should be
>> split up, or a patch in the middle has a typo), the fixes
>> shouldn't be
>> just heaped on top. The submitter should restructure
>> (rebase) the
>> series, and submit the reworked series on a new topic
>> branch / pull
>> request.
>>
>> Isn't this the contribution pattern we'd like to see in
>> all
>> sub-projects, where we (the TianoCore community) have
>> reviewer /
>> maintainer responsibilities?
>>
>> Thanks,
>> Laszlo
>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io
>> [mailto:devel@edk2.groups.io]
>>>> On Behalf Of Laszlo Ersek
>>>> Sent: Wednesday, May 8, 2019 2:56 AM
>>>> To: devel@edk2.groups.io; sean.brogan@microsoft.com
>>>> Subject: Re: [edk2-devel] RFC for Edk2-Library
>>>>
>>>> On 05/07/19 23:35, Sean via Groups.Io wrote:
>>>>> RFC  Edk2-Library creation
>>>>>
>>>>> Create a new tianocore owned repository to host a
>>>> python library
>>>>> package in support of UEFI development.  This package
>>>> will allow easy
>>>>> sharing of python library code to facilitate reuse.
>>>> Inclusion of this
>>>>> package and dependency management should be managed
>>>> using Pip/Pypi. To
>>>>> start this is a supplemental package and is not
>>>> required to be used
>>>>> for edk2 builds.
>>>>
>>>> [1]
>>>>
>>>>> Examples of content here
>>>>>
>>>>> * Edk2 file type parsing
>>>>> * UEFI structure encode/decode in python
>>>>> * Packaging tools (Capsules generation, signing, INF
>>>> gen, Cat gen)
>>>>> * TPM support code
>>>>> * Potentially move content from
>>>> basetools/source/python/common/*
>>>>
>>>> [2]
>>>>
>>>>> * No command line tools/interface
>>>>>
>>>>> Maintainers
>>>>>
>>>>> * Sean Brogan
>>>>> * Bret Barkelew
>>>>> * Placeholder for existing maintainer from the
>>>> basetools
>>>>>
>>>>> License
>>>>>
>>>>> * BSD + Patent (edk2 aligned)
>>>>>
>>>>> Contribution process and issue tracking
>>>>>
>>>>> * Follow Github PR process for contributions and
>> issue
>>>> tracking
>>>>> * Contributor forks repo in github
>>>>> * Contributor creates branch for work
>>>>> * Contributor updates release notes to indicate
>> change
>>>> (if necessary)
>>>>> * Contributor submits PR to master branch of
>>>> tianocore/Edk2-Library
>>>>>   repo
>>>>> * Review feedback is given in PR
>>>>> * Python Tests are run on the repo (new contributions
>>>> need unit tests)
>>>>> * Python Style (flake8) must pass
>>>>> * All review feedback must be completed, maintainers
>>>> approved, and
>>>>>   tests run successfully before PR is *squash merged*
>>>> into master
>>>>
>>>> The sentences
>>>>
>>>> [1] "To start this is a supplemental package and is
>> not
>>>> required to be
>>>>      used for edk2 builds."
>>>>
>>>> [2] "Potentially move content from
>>>> basetools/source/python/common/*"
>>>>
>>>> foreshadow that such a code movement might happen down
>>>> the road, and the
>>>> external package could become a requirement for
>> building
>>>> edk2.
>>>>
>>>> That step would mean the following:
>>>>
>>>> (a) Edk2 would not remain buildable from a single
>>>> command
>>>>
>>>>     git clone --recurse-submodules
>>>>
>>>>     Building edk2 would require GNU/Linux users to
>> start
>>>> tracking
>>>>     packages with "pip", which is independent of any
>>>> given distro's own
>>>>     package management, and may cause conflicts if not
>>>> used carefully:
>>>>
>>>>
>>>>
>> https://developer.fedoraproject.org/tech/languages/pytho
>>>> n/pypi-installation.html
>>>>
>>>>     This requirement on "pip" would only go away once
>>>> the external
>>>>     python dependencies were packaged for at least the
>>>> larger GNU/Linux
>>>>     distros.
>>>>
>>>> (b) Edk2 users running into build problems related to
>>>> the external
>>>>     python dependencies would have to contribute
>> through
>>>> a github-only
>>>>     workflow. That's not a deal-breaker per se -- if
>> we
>>>> want to
>>>>     contribute to other edk2 dependencies, such as
>>>> OpenSSL or nasm, we
>>>>     also have to conform to their specific development
>>>> models, clearly.
>>>>
>>>>     However, "squash merge" is a catastrophically bad
>>>> development model,
>>>>     and I'd object to introducing a new edk2 build
>>>> dependency that
>>>>     followed that model.
>>>>
>>>>     (There are other issues with the GitHub.com
>>>> development workflow, as
>>>>     discussed elsewhere, but "squash merge" takes the
>>>> cake.)
>>>>
>>>> Problem (a) would be solvable in the long term
>> (through
>>>> collaboration
>>>> with distro maintainers, i.e. downstream packaging),
>> but
>>>> problem (b)
>>>> would not be. Thus I'm fine with the proposal, in its
>>>> current form, only
>>>> if the new package is 100% an addition on top of edk2,
>>>> even in the long
>>>> term, and not in any part intended as a future
>>>> replacement for current
>>>> edk2 functionality.
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>> 
>>>
> 

Re: [edk2-devel] RFC for Edk2-Library

[Laszlo Ersek]

On 05/10/19 04:48, Sean via Groups.Io wrote:
> 1. Agree on the name but not sure whats better.  i don't think it
> should be edk2-tools because the idea of this is to be a library of
> support code but not the tools themselves.  This limits dependencies
> and keeps the library free of business specific logic.  The second RFC
> will be for a new repo that is more like edk2-tools.  So maybe this
> could be edk2-tools-library?
>
> 2. Future direction (edk2 having a dependency on this package).  I
> don't think this library will achieve my goals if it doesn't start to
> pull in support libraries from basetools.  The simplest example is the
> parsers.  It would seem foolish to have two copies.  It would be great
> to have one set of parsers that could be used by tool developers as
> well as the edk2 build process.  Opening up those tools would make
> writing edk2 analysis tools much easier/faster/better.
>
> 3. Pip/distribution/versioning.  I would definitely not want my
> version of this pip module dependent on my OS distro and version. This
> will be something tied to the edk2 platform i am building. These are
> things that are somewhat stable but i would expect more churn than an
> OS and different platforms will have different needs.  i also don't
> want to sign up for working with OS vendors to get this into their
> package management.  With python 3.5 and newer there is a built in
> concept of virtual environments (venv).  This is how i would handle
> this problem. The Pip modules and their dependencies do not impact the
> global packages.  Only the virtual environment gets updated and a
> virtual environment can be trivially created/deleted.  This is also
> expected if you are building platforms that might be running different
> versions of edk2 so its a good idea to create a venv for each code
> tree on your system and activate that venv when doing builds in that
> code tree.

OK. I think we'll just have to package the right version of the new
python dependency, downstream, eventually.


> 4. Squash merge/PR process.  We discussed this at length at plugfest
> and unfortunately i don't think there was any silver bullet. Patchsets
> and the edk2 process today are a relic of emailing patches.

That's your opinion ("relic"), which I respect. It's not a fact though.
There are large communities that email patches (with git-send-email)
because that's the most scalable and proven approach for them. It's not
because they are prevented from using anything better. For these people
(including me), there *is* nothing better.

I *have* contributed to projects with a GitHub.com-only workflow. Red
Hat contributes to, and maintains, several such projects. git-send-email
is still better, in my opinion. It's not perfect, of course.

If the TianoCore community perceives the patch email based workflow
limiting, I will not try to prevent the community from adopting
something else. But I would like to see some values preserved. (Unless
contributors clearly state that they don't care about those values.)


> When you move to pull requests managed by a server with policy and
> automation i think the only feasible solution is squash merge.  Now i
> don't understand why squash merge means bad commit messages and lost
> info.

Please refer to the following patch series, as an example.

  [edk2] [PATCH 0/4]
  MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

  http://mid.mail-archive.com/20180215183638.18578-1-lersek@redhat.com
  https://lists.01.org/pipermail/edk2-devel/2018-February/021476.html

It was committed as:

  1  54c7728a0465 MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub()
  2  41bfaffd1309 MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Add()
  3  8c33cc0ec926 MdePkg/BaseSafeIntLib: clean up parentheses in MIN_INT64_MAGNITUDE
  4  75505d161133 MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Mult()

If you squash these patches together, you will end up with a single
patch that modifies three independent functions, and rewrites a macro,
at the same time.

In addition, you will either lose the details of the individual commit
messages, or else you will end up with a humongous and incoherent commit
message. For example, the reasoning in commit #1 is distinct from the
reasoning in commit #4.

In addition, if the series had caused a regression, then git-bisect
would have had a lot worse granularity in locating the culprit patch.
With the above patch set structure, git-bisect would have told us what
function or macro to start investigating. Squashed together, we'd have
only gotten a *set* of functions to investigate.

This is not a theoretical problem. 1000-line feature patches are still
all too common in edk2. They are basically impossible to analyze in a
bug hunt, especially for someone that has not originally authored the
feature.

(Analyzing someone else's code for issues is the norm in open source
development.)


>  The idea is that a single PR should be a single feature.

Yes, I agree 100%.

And implementing a single feature may take tens of patches, which should
not be squashed together.


>  When it gets squashed the commit message should reflect the feature
>  and be of high quality.

Commit messages are entirely expected to go into implementation details.
A high level feature description is entirely justified and even
necessary, but it belongs in the feature request ticket, a wiki article,
and/or a standalone design document.


>  If the PR was too big and contained numerous features then the PR
>  should be split up.

Fully agreed. However, the above example justifies neither more than one
PR, nor squashing patches #1 through #4.


>  Squash merge allows your automation to easily guarantee
>  bisect-ability,

I don't understand. If I develop patches #1 through #4 logically
separated from each other, but then I push them as one big squashed
commit, how will any kind of automation conjure up the original patch
boundaries?


> build-ability, and that each commit will pass the requirements. Having
> one continuous PR optimizes your review process/comment tracking,
> etc.  I would be strongly opposed to opening new PRs for every new
> "version".

I don't insist on a new PR for every version of a patch series, as long
as every version of the patch series is preserved forever, as a
*complete* topic branch, with context-sensitive review comments.


I'd like contributors to edk2 (both proven and prospective) to express
their preferences on this. If it's demonstrated that I'm trying to force
workflow specifics on edk2 that are not welcome by the majority, I'll
shut up.

Thanks
Laszlo

>
> Thanks
> Sean
>
> 
>
>

Re: [edk2-devel] RFC for Edk2-Library

[Michael D Kinney]

Laszlo,

On Windows build systems, we have to install OpenSSL command line
utilities.  For all host systems, the OpenSSL command line
utilities need to be in the system path.  My point is that this 
is similar to other dependencies like iASL and NASM.

For the patch discussion, I did not mean to confuse things.  From
one perspective, there are two types of patch submissions.  Single
commit (no series) and multiple commits (patch series).  The squash
merge of a PR works just fine for the single commit (no series) type.
There may be feedback/comments that require code changes and once the 
PR is accepted, the history in the repository shows a single commit.
As the PR evolves, commits are made on the PR to address each piece
of feedback.  Squashing all of these together at the time the PR is
accepted is the correct action and matches what we do the for email
based review process.  The final result for both PR and email is a
single commit with a cleaned up commit message.

You may consider the single commit (no series) type more rare than the
multiple commit (patch series) type.  However, there may be cases where
a multiple commit (patch series) was used where the changes could have
been submitted as a set of single commit (no series) changes.

Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Laszlo Ersek
> Sent: Monday, May 13, 2019 3:46 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; sean.brogan@microsoft.com
> Subject: Re: [edk2-devel] RFC for Edk2-Library
> 
> On 05/10/19 02:01, Kinney, Michael D wrote:
> > Laszlo,
> >
> > 1) We also use OpenSSL command line tool to locally
> sign
> >    capsules and recovery images for local testing.  So
> both
> >    tool dependency and source dependency apply to the
> OpenSSL
> >    content.
> 
> I haven't used the tools yet that you refer to, so I'm
> unsure how
> exactly they invoke the "openssl" utility. If they just
> rely on the PATH
> environment variable, then what they invoke comes from
> the system-wide
> openssl package.
> 
> >
> > 2) If a dev submits a PR, and there are many review
> comments
> >    that require code changes, then those code changes
> are
> >    added to that PR until all feedback is addressed.
> 
> I don't understand how. Let's say the pull request refers
> to a branch
> with three commits, and the majority of the review
> comments request
> updates for patch #2 (i.e., in the middle). How can the
> submitter "add
> changes to the PR"? It is patch #2 that needs to be
> reworked, which will
> require rebases, and so the hash of the HEAD commit of
> the topic branch
> (patch #3) will change as well.
> 
> >    At that
> >    point, if the change is something that should be in
> a single
> >    commit, then doing a squash merge with a cleaned up
> commit
> >    message would be appropriate.
> 
> I don't understand -- we modify only patch #2, yes, to
> address review
> comments, but why does that justify squashing #1 through
> #3 into a
> single commit?
> 
> >    And the history of all the
> >    review feedback preserved in the PR.
> 
> That's good (but it could be better -- see the lacking
> email
> integration. Anyway this is not strictly tied to my
> concern with
> squash-on-merge).
> 
> >
> >    If we create a 2nd PR with the cleaned up content,
> then the
> >    connection to the 1st PR feedback may be lost.  I
> agree this
> >    matches what we do in email reviews to create a V2
> patch series.
> >    The V1 and V2 threads are in the email archive.
> 
> Indeed. And the blurb for both threads reference the
> bugzilla, and the
> bugzilla has (if the submitter is careful) comments
> linking the v1 and
> v2 threads from the email archive.
> 
> >    I wonder if
> >    there is a way to link a 2nd PR to the 1st PR and
> guarantee
> >    that both PRs are preserved?  This would also allow
> the 2nd
> >    cleaned up PR to contain a series, and if there was
> a way to
> >    make the squash merge optional, then the developer
> can choose
> >    the way the patches are committed.
> 
> This sounds great! I think both PRs can reference the
> bugzilla ticket,
> and the bugzilla ticket can reference both PRs too (as
> comments). The
> first PR can be rejected & closed when the second one is
> submitted.
> 
> >    Just a few ideas to explore...Perhaps Sean can
> provide some
> >    additional ideas to manage complex changes in PRs.
> 
> Thanks!
> Laszlo
> 
> >
> > Best regards,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Thursday, May 9, 2019 3:56 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> >> devel@edk2.groups.io; sean.brogan@microsoft.com
> >> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>
> >> On 05/09/19 20:06, Kinney, Michael D wrote:
> >>> Hello,
> >>>
> >>> It is difficult to tell if the repo name edk2-library
> >> is for firmware
> >>> or tools, so I recommend we work on a name that
> clearly
> >> identifies
> >>> that this repo is related to tools.
> >>
> >> Good idea.
> >>
> >>> For the pip dependencies, is the concern that a
> >> platform that depends
> >>> on these tools will not be buildable without running
> a
> >> "pip install"
> >>> command that pulls content from the network?
> >>
> >> Almost. Not exactly.
> >>
> >> First, the concern is not specific to any given
> platform.
> >> My
> >> understanding is that the extraction would target, in
> the
> >> longer term,
> >> general BaseTools functionality. That would impact all
> >> platforms that
> >> consume & build against edk2.
> >>
> >> Second, it's not the network access that's concerning
> per
> >> se. It's the
> >> compatibility / interoperation with any given Linux
> >> distribution's own
> >> (native) package management system. Such a package
> >> management system is
> >> generally not used on Windows, therefore Windows users
> >> are accustomed to
> >> installing software packages from a boatload of
> vendors.
> >> This is not the
> >> case on Linux distros -- your distro vendor offers a
> >> curated set of
> >> packages, such that everything interoperates with
> >> everything (ideally),
> >> there are no file conflicts, version dependencies are
> >> handled
> >> automatically (e.g. if you install a high-level
> package,
> >> its
> >> dependencies are pulled in automatically), and so on.
> >>
> >> Clearly the distro vendor does not *author* all this
> >> software (although
> >> they are strongly encouraged to contribute to the
> >> upstream software
> >> projects they package and ship). The distro vendor is
> >> responsible for
> >> the integration of all these packages into a
> consistent
> >> OS, into
> >> consistent feature sets, and so on.
> >>
> >> "pip" and similar tools are generally unfit for this
> >> approach, because
> >> they implement a parallel package management system,
> to
> >> my
> >> understanding. If they are carefully used in a user's
> >> home directory,
> >> they might work. If packages were installed with "pip"
> >> system-wide, they
> >> would almost certainly break (conflict with) the
> distro-
> >> provided
> >> packages.
> >>
> >> Therefore, when users would like to get a new piece of
> >> software
> >> packaged, or an existent package refreshed (to a more
> >> recent upstream
> >> version), they file a feature requst with their distro
> >> vendor (unless
> >> the distro vendor already tracks the upstream project
> >> closely and
> >> performs periodic rebases anyway). Then the distro
> vendor
> >> ships a new
> >> (or refreshed) package, again nicely integrated with
> the
> >> rest of the
> >> system.
> >>
> >>> We already have to pull content to get the sources
> and
> >> potentially
> >>> other dependent tools (NASM, iASL, openssl).
> >>
> >> Yes, these are good examples to demonstrate the
> >> difference. Consider
> >> NASM. If a Windows user would like to install NASM,
> they
> >> google "NASM
> >> for Windows", then go to:
> >>
> >>
> https://www.nasm.us/pub/nasm/releasebuilds/2.14.02/win64/
> >>
> >> download the ZIP file or EXE file, and install it.
> >>
> >> In comparison, a Fedora user runs
> >>
> >>   dnf install nasm
> >>
> >> and they get the package from the Fedora package
> >> repository. A Debian
> >> user would run
> >>
> >>   apt-get install nasm
> >>
> >> and they would get the package (which would be
> entirely
> >> independent of
> >> Fedora's) from the Debian package repository.
> >>
> >> The various *BSDs would run a variant of
> >>
> >>   pkg_add nasm
> >>
> >> I believe.
> >>
> >> The same applies to iASL.
> >>
> >>
> >> OpenSSL is a special case. It is different from NASM
> and
> >> iASL. NASM and
> >> iASL are native (hosted) applications, so any Linux
> >> distro can easily
> >> build them (for itself), and the binaries can be
> shipped
> >> to users, ready
> >> to run. But for the purposes of edk2, OpenSSL is not
> >> needed as a native
> >> (hosted) utility, like NASM and iASL -- it is needed
> as a
> >> *cross-built*
> >> static library, targeting the firmware architecture
> (not
> >> the host
> >> architecture). Unfortunately, OpenSSL (the upstream
> >> project) does not
> >> fully support this use case yet, therefore Linux
> distros
> >> cannot just
> >> cross-compile OpenSSL for edk2, and offer static
> library
> >> packages. This
> >> is why upstream edk2 consumes OpenSSL through a git
> >> submodule, in source
> >> form.
> >>
> >> *However*: conceptually, there is no difference. In
> >> Fedora and RHEL, we
> >> don't build OVMF against upstream OpenSSL. We build it
> >> against the
> >> downstream OpenSSL source code. So ultimately there is
> no
> >> difference in
> >> the distro vendor's approach -- the sources and the
> >> binaries come from
> >> the distro vendor.
> >>
> >>
> >> So what follows from this? It means that, for using
> >> BaseTools on a Linux
> >> distro, users will have to install some extra Python
> >> packages, in
> >> addition to NASM and iASL, from their vendor's
> >> repository. In other
> >> words, the "edk2-library" (or "edk2-tools") project
> >> should do upstream
> >> releases, so that Linux distros can package them in
> their
> >> native package
> >> systems. And some collaboration between upstream and
> >> downstreams is
> >> expected for this. (Exactly as it occurs with the iASL
> >> and NASM upstream
> >> projects.)
> >>
> >>> Can we limit the initial scope to tools that layer on
> >> top of edk2, and
> >>> a different future RFC would be required if there is
> a
> >> proposal for
> >>> the edk2 repo to depend on another repo?
> >>
> >> Yes, that would be very nice.
> >>
> >>> If we accept this limited scope, are there still
> >> concerns about
> >>> initially using squash commits for changes to these
> >> tools.
> >>
> >> Yes, I still have the same concern -- although it's a
> >> milder concern,
> >> dependent on how long squash merges are tolerated.
> >>
> >> Assume that a second, separate RFC gets posted down
> the
> >> road, let's say
> >> a year in, which proposes to replace the in-line
> >> component FooBar of
> >> BaseTools, with the external project "edk2-library"
> (or
> >> "edk2-tools").
> >> At that point, the git history accrued thus far, in
> edk2-
> >> tools, would
> >> suddenly be inherited by all BaseTools users. And that
> >> history wouldn't
> >> be really helpful.
> >>
> >> For an example, we need not look past the edk2
> repository
> >> itself. Let's
> >> say I'd like to learn about the implementation history
> of
> >> the
> >> HandleProtocol() boot service, in edk2. So I run:
> >>
> >> $ git blame master --
> MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>
> >> and I look up the CoreHandleProtocol() function, and
> find
> >> this:
> >>
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  931)
> >> EFI_STATUS
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  932)
> EFIAPI
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  933)
> >> CoreHandleProtocol (
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  934)   IN
> >> EFI_HANDLE       UserHandle,
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  935)   IN
> >> EFI_GUID         *Protocol,
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  936)
> OUT
> >> VOID            **Interface
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  937)   )
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  938) {
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  939)
> >> return CoreOpenProtocol (
> >>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >> (qhuang8          2008-07-24 02:54:45 +0000  940)
> >> UserHandle,
> >>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >> (qhuang8          2008-07-24 02:54:45 +0000  941)
> >> Protocol,
> >>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >> (qhuang8          2008-07-24 02:54:45 +0000  942)
> >> Interface,
> >>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >> (qhuang8          2008-07-24 02:54:45 +0000  943)
> >> gDxeCoreImageHandle,
> >>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >> (qhuang8          2008-07-24 02:54:45 +0000  944)
> >> NULL,
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  945)
> >> EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  946)
> >> );
> >>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >> (yshang1          2007-07-04 10:51:54 +0000  947) }
> >>
> >> Interesting, it seems like the function was originally
> >> added in commit
> >> 28a00297189c3, and then the argument list was modified
> in
> >> commit
> >> 022c6d45ef786, a year later. So let's check out the
> >> rationale for that
> >> argument list update:
> >>
> >> $ git show --color 022c6d45ef786
> >>
> >> Well... The commit message says, "Code Scrub for Dxe
> >> Core". That's all.
> >> We also learn it comes from SVN revision 5560. And the
> >> diffstat for the
> >> patch is:
> >>
> >>>  38 files changed, 2499 insertions(+), 2504
> deletions(-
> >> )
> >>
> >> It is not overly helpful.
> >>
> >> Clearly, CoreHandleProtocol() is a super-robust,
> super-
> >> mature function,
> >> so we don't really care for its development history
> >> today.
> >>
> >> I believe the same would not apply to the "edk2-
> library"
> >> (or
> >> "edk2-tools") project. When it replaced the in-line
> >> component FooBar of
> >> BaseTools, it's plausible it would be suddenly exposed
> to
> >> a larger
> >> userbase. New issues would be encountered, and people
> >> might want to look
> >> at the git history -- if for nothing else, then
> rationale
> >> (commit
> >> messages) on small parts of the code.
> >>
> >>
> >> I can also name more recent examples. I'm not a
> frequent
> >> BaseTools
> >> contributor, but it has happened occasionally. (I've
> got
> >> 41 patches in
> >> the edk2 git history that touched BaseTools/, as of
> >> commit
> >> 0a506fc7ab8b.)
> >>
> >> * For
> >> <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>,
> I
> >> pushed
> >>   5+1 patches:
> >>
> >>     67983484a443 BaseTools/footer.makefile: expand
> >> BUILD_CFLAGS last for C files too
> >>     03252ae287c4 BaseTools/header.makefile: remove "-
> c"
> >> from BUILD_CFLAGS
> >>     b8a661702643 BaseTools/Source/C: split "-O2" to
> >> BUILD_OPTFLAGS
> >>     b0ca5dae78ff BaseTools/Source/C: take
> EXTRA_OPTFLAGS
> >> from the caller
> >>     81502cee20ac BaseTools/Source/C: take
> EXTRA_LDFLAGS
> >> from the caller
> >>     +
> >>     aa4e0df1f0c7 BaseTools/VfrCompile: honor
> >> EXTRA_LDFLAGS
> >>
> >> * For
> >> <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>,
> I
> >> pushed 26
> >>   patches:
> >>
> >>     8ff122119941 EmulatorPkg: require GCC48 or later
> >>     8d7cdfae8cb8 OvmfPkg: require GCC48 or later
> >>     fd158437dcce Vlv2TbltDevicePkg: assume GCC48 or
> later
> >>     7a9dbf2c94d1 BaseTools/Conf/tools_def.template:
> drop
> >> ARM/AARCH support from GCC46/GCC47
> >>     48e64498c961 BaseTools/tools_def.template: fix up
> LF-
> >> only line terminator
> >>     7381a6627a66 BaseTools/tools_def.template: strip
> >> trailing whitespace
> >>     9bbf156faaad BaseTools/tools_def.template: remove
> >> GCC48_IA32_X64_DLINK_COMMON dead-end
> >>     3c5613c5932c BaseTools/tools_def.template: remove
> >> GCC47 leaf definitions
> >>     fc87b8d7f411 BaseTools/tools_def.template:
> propagate
> >> loss of GCC47 references
> >>     91a67e0f111e BaseTools/tools_def.template: remove
> >> GCC47 documentation
> >>     0f234fb8a662 BaseTools/tools_def.template: remove
> >> GCC46 leaf definitions
> >>     83a8f313884a BaseTools/tools_def.template:
> propagate
> >> loss of GCC46 references
> >>     be359fa7ceec BaseTools/tools_def.template: remove
> >> GCC46 documentation
> >>     1458af0cbce0 BaseTools/tools_def.template: remove
> >> GCC45 leaf definitions
> >>     024576896d42 BaseTools/tools_def.template:
> propagate
> >> loss of GCC45 references
> >>     3e77d20f5cb3 BaseTools/tools_def.template: remove
> >> GCC45 documentation
> >>     e046dc60fb89 BaseTools/tools_def.template: remove
> >> GCC44 leaf definitions
> >>     38c570efede0 BaseTools/tools_def.template:
> propagate
> >> loss of GCC44 references
> >>     383d29096846 BaseTools/tools_def.template: rename
> >> GCC44_ALL_CC_FLAGS to GCC48_ALL_CC_FLAGS
> >>     0db91daf5228 BaseTools/tools_def.template:
> eliminate
> >> GCC44_IA32_X64_DLINK_FLAGS
> >>     84d21abf4e36 BaseTools/tools_def.template: rename
> >> GCC44_IA32_X64_DLINK_COMMON to
> >> GCC48_IA32_X64_DLINK_COMMON
> >>     5c6ccd53244b BaseTools/tools_def.template: remove
> >> comment about GCC44 + LzmaF86Compress
> >>     3bc65326d6ed BaseTools/tools_def.template: remove
> >> GCC44 documentation
> >>     f7282023e758 ArmPkg/ArmSoftFloatLib: drop build
> flags
> >> specific to GCC46/GCC47
> >>     300b8c5f150c CryptoPkg/BaseCryptLib: drop build
> flags
> >> specific to GCC44
> >>     7423ba9d499b Revert "MdePkg: avoid
> >> __builtin_unreachable() on GCC v4.4"
> >>
> >>   (Note, from this list, 7a9dbf2c94d1 was authored by
> >> Ard, I just
> >>   included it at the right spot.)
> >>
> >> All of these patch series were carefully structured,
> and
> >> carefully
> >> documented (in the commit messages). I would have been
> >> devastated, had
> >> they been squashed.
> >>
> >> (And it would have been questionable to squash Ard's
> >> patch with patches
> >> authored by me.)
> >>
> >>
> >>> Is there a way for GitHub to support both squash
> >> commits for some PRs
> >>> and preserve a patch series for other PRs?
> >>
> >> I don't know.
> >>
> >> But honestly, what is the purpose of squash merges?
> Don't
> >> we expect
> >> *all* contributions in well structured patch series?
> >>
> >> If review discovers an issue with the structure of the
> >> series (for
> >> example, not buildable in the middle, or the steps are
> >> not logical in
> >> that order, or some patches do too many things at once
> >> and should be
> >> split up, or a patch in the middle has a typo), the
> fixes
> >> shouldn't be
> >> just heaped on top. The submitter should restructure
> >> (rebase) the
> >> series, and submit the reworked series on a new topic
> >> branch / pull
> >> request.
> >>
> >> Isn't this the contribution pattern we'd like to see
> in
> >> all
> >> sub-projects, where we (the TianoCore community) have
> >> reviewer /
> >> maintainer responsibilities?
> >>
> >> Thanks,
> >> Laszlo
> >>
> >>> Thanks,
> >>>
> >>> Mike
> >>>
> >>>> -----Original Message-----
> >>>> From: devel@edk2.groups.io
> >> [mailto:devel@edk2.groups.io]
> >>>> On Behalf Of Laszlo Ersek
> >>>> Sent: Wednesday, May 8, 2019 2:56 AM
> >>>> To: devel@edk2.groups.io; sean.brogan@microsoft.com
> >>>> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>>>
> >>>> On 05/07/19 23:35, Sean via Groups.Io wrote:
> >>>>> RFC  Edk2-Library creation
> >>>>>
> >>>>> Create a new tianocore owned repository to host a
> >>>> python library
> >>>>> package in support of UEFI development.  This
> package
> >>>> will allow easy
> >>>>> sharing of python library code to facilitate reuse.
> >>>> Inclusion of this
> >>>>> package and dependency management should be managed
> >>>> using Pip/Pypi. To
> >>>>> start this is a supplemental package and is not
> >>>> required to be used
> >>>>> for edk2 builds.
> >>>>
> >>>> [1]
> >>>>
> >>>>> Examples of content here
> >>>>>
> >>>>> * Edk2 file type parsing
> >>>>> * UEFI structure encode/decode in python
> >>>>> * Packaging tools (Capsules generation, signing,
> INF
> >>>> gen, Cat gen)
> >>>>> * TPM support code
> >>>>> * Potentially move content from
> >>>> basetools/source/python/common/*
> >>>>
> >>>> [2]
> >>>>
> >>>>> * No command line tools/interface
> >>>>>
> >>>>> Maintainers
> >>>>>
> >>>>> * Sean Brogan
> >>>>> * Bret Barkelew
> >>>>> * Placeholder for existing maintainer from the
> >>>> basetools
> >>>>>
> >>>>> License
> >>>>>
> >>>>> * BSD + Patent (edk2 aligned)
> >>>>>
> >>>>> Contribution process and issue tracking
> >>>>>
> >>>>> * Follow Github PR process for contributions and
> >> issue
> >>>> tracking
> >>>>> * Contributor forks repo in github
> >>>>> * Contributor creates branch for work
> >>>>> * Contributor updates release notes to indicate
> >> change
> >>>> (if necessary)
> >>>>> * Contributor submits PR to master branch of
> >>>> tianocore/Edk2-Library
> >>>>>   repo
> >>>>> * Review feedback is given in PR
> >>>>> * Python Tests are run on the repo (new
> contributions
> >>>> need unit tests)
> >>>>> * Python Style (flake8) must pass
> >>>>> * All review feedback must be completed,
> maintainers
> >>>> approved, and
> >>>>>   tests run successfully before PR is *squash
> merged*
> >>>> into master
> >>>>
> >>>> The sentences
> >>>>
> >>>> [1] "To start this is a supplemental package and is
> >> not
> >>>> required to be
> >>>>      used for edk2 builds."
> >>>>
> >>>> [2] "Potentially move content from
> >>>> basetools/source/python/common/*"
> >>>>
> >>>> foreshadow that such a code movement might happen
> down
> >>>> the road, and the
> >>>> external package could become a requirement for
> >> building
> >>>> edk2.
> >>>>
> >>>> That step would mean the following:
> >>>>
> >>>> (a) Edk2 would not remain buildable from a single
> >>>> command
> >>>>
> >>>>     git clone --recurse-submodules
> >>>>
> >>>>     Building edk2 would require GNU/Linux users to
> >> start
> >>>> tracking
> >>>>     packages with "pip", which is independent of any
> >>>> given distro's own
> >>>>     package management, and may cause conflicts if
> not
> >>>> used carefully:
> >>>>
> >>>>
> >>>>
> >>
> https://developer.fedoraproject.org/tech/languages/pytho
> >>>> n/pypi-installation.html
> >>>>
> >>>>     This requirement on "pip" would only go away
> once
> >>>> the external
> >>>>     python dependencies were packaged for at least
> the
> >>>> larger GNU/Linux
> >>>>     distros.
> >>>>
> >>>> (b) Edk2 users running into build problems related
> to
> >>>> the external
> >>>>     python dependencies would have to contribute
> >> through
> >>>> a github-only
> >>>>     workflow. That's not a deal-breaker per se -- if
> >> we
> >>>> want to
> >>>>     contribute to other edk2 dependencies, such as
> >>>> OpenSSL or nasm, we
> >>>>     also have to conform to their specific
> development
> >>>> models, clearly.
> >>>>
> >>>>     However, "squash merge" is a catastrophically
> bad
> >>>> development model,
> >>>>     and I'd object to introducing a new edk2 build
> >>>> dependency that
> >>>>     followed that model.
> >>>>
> >>>>     (There are other issues with the GitHub.com
> >>>> development workflow, as
> >>>>     discussed elsewhere, but "squash merge" takes
> the
> >>>> cake.)
> >>>>
> >>>> Problem (a) would be solvable in the long term
> >> (through
> >>>> collaboration
> >>>> with distro maintainers, i.e. downstream packaging),
> >> but
> >>>> problem (b)
> >>>> would not be. Thus I'm fine with the proposal, in
> its
> >>>> current form, only
> >>>> if the new package is 100% an addition on top of
> edk2,
> >>>> even in the long
> >>>> term, and not in any part intended as a future
> >>>> replacement for current
> >>>> edk2 functionality.
> >>>>
> >>>> Thanks,
> >>>> Laszlo
> >>>>
> >>>>
> >>>
> >
> 
> 
> 

Re: [edk2-devel] RFC for Edk2-Library

[Laszlo Ersek]

On 05/13/19 20:20, Kinney, Michael D wrote:
> Laszlo,
> 
> On Windows build systems, we have to install OpenSSL command line
> utilities.  For all host systems, the OpenSSL command line
> utilities need to be in the system path.  My point is that this 
> is similar to other dependencies like iASL and NASM.

OK. I think I must have misunderstood you at some point. Sorry about that.

> For the patch discussion, I did not mean to confuse things.  From
> one perspective, there are two types of patch submissions.  Single
> commit (no series) and multiple commits (patch series).  The squash
> merge of a PR works just fine for the single commit (no series) type.
> There may be feedback/comments that require code changes and once the 
> PR is accepted, the history in the repository shows a single commit.
> As the PR evolves, commits are made on the PR to address each piece
> of feedback.  Squashing all of these together at the time the PR is
> accepted is the correct action and matches what we do the for email
> based review process.  The final result for both PR and email is a
> single commit with a cleaned up commit message.

OK.

> You may consider the single commit (no series) type more rare than the
> multiple commit (patch series) type.  However, there may be cases where
> a multiple commit (patch series) was used where the changes could have
> been submitted as a set of single commit (no series) changes.

OK.

Thanks
Laszlo

> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
>> On Behalf Of Laszlo Ersek
>> Sent: Monday, May 13, 2019 3:46 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> devel@edk2.groups.io; sean.brogan@microsoft.com
>> Subject: Re: [edk2-devel] RFC for Edk2-Library
>>
>> On 05/10/19 02:01, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> 1) We also use OpenSSL command line tool to locally
>> sign
>>>    capsules and recovery images for local testing.  So
>> both
>>>    tool dependency and source dependency apply to the
>> OpenSSL
>>>    content.
>>
>> I haven't used the tools yet that you refer to, so I'm
>> unsure how
>> exactly they invoke the "openssl" utility. If they just
>> rely on the PATH
>> environment variable, then what they invoke comes from
>> the system-wide
>> openssl package.
>>
>>>
>>> 2) If a dev submits a PR, and there are many review
>> comments
>>>    that require code changes, then those code changes
>> are
>>>    added to that PR until all feedback is addressed.
>>
>> I don't understand how. Let's say the pull request refers
>> to a branch
>> with three commits, and the majority of the review
>> comments request
>> updates for patch #2 (i.e., in the middle). How can the
>> submitter "add
>> changes to the PR"? It is patch #2 that needs to be
>> reworked, which will
>> require rebases, and so the hash of the HEAD commit of
>> the topic branch
>> (patch #3) will change as well.
>>
>>>    At that
>>>    point, if the change is something that should be in
>> a single
>>>    commit, then doing a squash merge with a cleaned up
>> commit
>>>    message would be appropriate.
>>
>> I don't understand -- we modify only patch #2, yes, to
>> address review
>> comments, but why does that justify squashing #1 through
>> #3 into a
>> single commit?
>>
>>>    And the history of all the
>>>    review feedback preserved in the PR.
>>
>> That's good (but it could be better -- see the lacking
>> email
>> integration. Anyway this is not strictly tied to my
>> concern with
>> squash-on-merge).
>>
>>>
>>>    If we create a 2nd PR with the cleaned up content,
>> then the
>>>    connection to the 1st PR feedback may be lost.  I
>> agree this
>>>    matches what we do in email reviews to create a V2
>> patch series.
>>>    The V1 and V2 threads are in the email archive.
>>
>> Indeed. And the blurb for both threads reference the
>> bugzilla, and the
>> bugzilla has (if the submitter is careful) comments
>> linking the v1 and
>> v2 threads from the email archive.
>>
>>>    I wonder if
>>>    there is a way to link a 2nd PR to the 1st PR and
>> guarantee
>>>    that both PRs are preserved?  This would also allow
>> the 2nd
>>>    cleaned up PR to contain a series, and if there was
>> a way to
>>>    make the squash merge optional, then the developer
>> can choose
>>>    the way the patches are committed.
>>
>> This sounds great! I think both PRs can reference the
>> bugzilla ticket,
>> and the bugzilla ticket can reference both PRs too (as
>> comments). The
>> first PR can be rejected & closed when the second one is
>> submitted.
>>
>>>    Just a few ideas to explore...Perhaps Sean can
>> provide some
>>>    additional ideas to manage complex changes in PRs.
>>
>> Thanks!
>> Laszlo
>>
>>>
>>> Best regards,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Thursday, May 9, 2019 3:56 PM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>>>> devel@edk2.groups.io; sean.brogan@microsoft.com
>>>> Subject: Re: [edk2-devel] RFC for Edk2-Library
>>>>
>>>> On 05/09/19 20:06, Kinney, Michael D wrote:
>>>>> Hello,
>>>>>
>>>>> It is difficult to tell if the repo name edk2-library
>>>> is for firmware
>>>>> or tools, so I recommend we work on a name that
>> clearly
>>>> identifies
>>>>> that this repo is related to tools.
>>>>
>>>> Good idea.
>>>>
>>>>> For the pip dependencies, is the concern that a
>>>> platform that depends
>>>>> on these tools will not be buildable without running
>> a
>>>> "pip install"
>>>>> command that pulls content from the network?
>>>>
>>>> Almost. Not exactly.
>>>>
>>>> First, the concern is not specific to any given
>> platform.
>>>> My
>>>> understanding is that the extraction would target, in
>> the
>>>> longer term,
>>>> general BaseTools functionality. That would impact all
>>>> platforms that
>>>> consume & build against edk2.
>>>>
>>>> Second, it's not the network access that's concerning
>> per
>>>> se. It's the
>>>> compatibility / interoperation with any given Linux
>>>> distribution's own
>>>> (native) package management system. Such a package
>>>> management system is
>>>> generally not used on Windows, therefore Windows users
>>>> are accustomed to
>>>> installing software packages from a boatload of
>> vendors.
>>>> This is not the
>>>> case on Linux distros -- your distro vendor offers a
>>>> curated set of
>>>> packages, such that everything interoperates with
>>>> everything (ideally),
>>>> there are no file conflicts, version dependencies are
>>>> handled
>>>> automatically (e.g. if you install a high-level
>> package,
>>>> its
>>>> dependencies are pulled in automatically), and so on.
>>>>
>>>> Clearly the distro vendor does not *author* all this
>>>> software (although
>>>> they are strongly encouraged to contribute to the
>>>> upstream software
>>>> projects they package and ship). The distro vendor is
>>>> responsible for
>>>> the integration of all these packages into a
>> consistent
>>>> OS, into
>>>> consistent feature sets, and so on.
>>>>
>>>> "pip" and similar tools are generally unfit for this
>>>> approach, because
>>>> they implement a parallel package management system,
>> to
>>>> my
>>>> understanding. If they are carefully used in a user's
>>>> home directory,
>>>> they might work. If packages were installed with "pip"
>>>> system-wide, they
>>>> would almost certainly break (conflict with) the
>> distro-
>>>> provided
>>>> packages.
>>>>
>>>> Therefore, when users would like to get a new piece of
>>>> software
>>>> packaged, or an existent package refreshed (to a more
>>>> recent upstream
>>>> version), they file a feature requst with their distro
>>>> vendor (unless
>>>> the distro vendor already tracks the upstream project
>>>> closely and
>>>> performs periodic rebases anyway). Then the distro
>> vendor
>>>> ships a new
>>>> (or refreshed) package, again nicely integrated with
>> the
>>>> rest of the
>>>> system.
>>>>
>>>>> We already have to pull content to get the sources
>> and
>>>> potentially
>>>>> other dependent tools (NASM, iASL, openssl).
>>>>
>>>> Yes, these are good examples to demonstrate the
>>>> difference. Consider
>>>> NASM. If a Windows user would like to install NASM,
>> they
>>>> google "NASM
>>>> for Windows", then go to:
>>>>
>>>>
>> https://www.nasm.us/pub/nasm/releasebuilds/2.14.02/win64/
>>>>
>>>> download the ZIP file or EXE file, and install it.
>>>>
>>>> In comparison, a Fedora user runs
>>>>
>>>>   dnf install nasm
>>>>
>>>> and they get the package from the Fedora package
>>>> repository. A Debian
>>>> user would run
>>>>
>>>>   apt-get install nasm
>>>>
>>>> and they would get the package (which would be
>> entirely
>>>> independent of
>>>> Fedora's) from the Debian package repository.
>>>>
>>>> The various *BSDs would run a variant of
>>>>
>>>>   pkg_add nasm
>>>>
>>>> I believe.
>>>>
>>>> The same applies to iASL.
>>>>
>>>>
>>>> OpenSSL is a special case. It is different from NASM
>> and
>>>> iASL. NASM and
>>>> iASL are native (hosted) applications, so any Linux
>>>> distro can easily
>>>> build them (for itself), and the binaries can be
>> shipped
>>>> to users, ready
>>>> to run. But for the purposes of edk2, OpenSSL is not
>>>> needed as a native
>>>> (hosted) utility, like NASM and iASL -- it is needed
>> as a
>>>> *cross-built*
>>>> static library, targeting the firmware architecture
>> (not
>>>> the host
>>>> architecture). Unfortunately, OpenSSL (the upstream
>>>> project) does not
>>>> fully support this use case yet, therefore Linux
>> distros
>>>> cannot just
>>>> cross-compile OpenSSL for edk2, and offer static
>> library
>>>> packages. This
>>>> is why upstream edk2 consumes OpenSSL through a git
>>>> submodule, in source
>>>> form.
>>>>
>>>> *However*: conceptually, there is no difference. In
>>>> Fedora and RHEL, we
>>>> don't build OVMF against upstream OpenSSL. We build it
>>>> against the
>>>> downstream OpenSSL source code. So ultimately there is
>> no
>>>> difference in
>>>> the distro vendor's approach -- the sources and the
>>>> binaries come from
>>>> the distro vendor.
>>>>
>>>>
>>>> So what follows from this? It means that, for using
>>>> BaseTools on a Linux
>>>> distro, users will have to install some extra Python
>>>> packages, in
>>>> addition to NASM and iASL, from their vendor's
>>>> repository. In other
>>>> words, the "edk2-library" (or "edk2-tools") project
>>>> should do upstream
>>>> releases, so that Linux distros can package them in
>> their
>>>> native package
>>>> systems. And some collaboration between upstream and
>>>> downstreams is
>>>> expected for this. (Exactly as it occurs with the iASL
>>>> and NASM upstream
>>>> projects.)
>>>>
>>>>> Can we limit the initial scope to tools that layer on
>>>> top of edk2, and
>>>>> a different future RFC would be required if there is
>> a
>>>> proposal for
>>>>> the edk2 repo to depend on another repo?
>>>>
>>>> Yes, that would be very nice.
>>>>
>>>>> If we accept this limited scope, are there still
>>>> concerns about
>>>>> initially using squash commits for changes to these
>>>> tools.
>>>>
>>>> Yes, I still have the same concern -- although it's a
>>>> milder concern,
>>>> dependent on how long squash merges are tolerated.
>>>>
>>>> Assume that a second, separate RFC gets posted down
>> the
>>>> road, let's say
>>>> a year in, which proposes to replace the in-line
>>>> component FooBar of
>>>> BaseTools, with the external project "edk2-library"
>> (or
>>>> "edk2-tools").
>>>> At that point, the git history accrued thus far, in
>> edk2-
>>>> tools, would
>>>> suddenly be inherited by all BaseTools users. And that
>>>> history wouldn't
>>>> be really helpful.
>>>>
>>>> For an example, we need not look past the edk2
>> repository
>>>> itself. Let's
>>>> say I'd like to learn about the implementation history
>> of
>>>> the
>>>> HandleProtocol() boot service, in edk2. So I run:
>>>>
>>>> $ git blame master --
>> MdeModulePkg/Core/Dxe/Hand/Handle.c
>>>>
>>>> and I look up the CoreHandleProtocol() function, and
>> find
>>>> this:
>>>>
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  931)
>>>> EFI_STATUS
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  932)
>> EFIAPI
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  933)
>>>> CoreHandleProtocol (
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  934)   IN
>>>> EFI_HANDLE       UserHandle,
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  935)   IN
>>>> EFI_GUID         *Protocol,
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  936)
>> OUT
>>>> VOID            **Interface
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  937)   )
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  938) {
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  939)
>>>> return CoreOpenProtocol (
>>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>>>> (qhuang8          2008-07-24 02:54:45 +0000  940)
>>>> UserHandle,
>>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>>>> (qhuang8          2008-07-24 02:54:45 +0000  941)
>>>> Protocol,
>>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>>>> (qhuang8          2008-07-24 02:54:45 +0000  942)
>>>> Interface,
>>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>>>> (qhuang8          2008-07-24 02:54:45 +0000  943)
>>>> gDxeCoreImageHandle,
>>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
>>>> (qhuang8          2008-07-24 02:54:45 +0000  944)
>>>> NULL,
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  945)
>>>> EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  946)
>>>> );
>>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
>>>> (yshang1          2007-07-04 10:51:54 +0000  947) }
>>>>
>>>> Interesting, it seems like the function was originally
>>>> added in commit
>>>> 28a00297189c3, and then the argument list was modified
>> in
>>>> commit
>>>> 022c6d45ef786, a year later. So let's check out the
>>>> rationale for that
>>>> argument list update:
>>>>
>>>> $ git show --color 022c6d45ef786
>>>>
>>>> Well... The commit message says, "Code Scrub for Dxe
>>>> Core". That's all.
>>>> We also learn it comes from SVN revision 5560. And the
>>>> diffstat for the
>>>> patch is:
>>>>
>>>>>  38 files changed, 2499 insertions(+), 2504
>> deletions(-
>>>> )
>>>>
>>>> It is not overly helpful.
>>>>
>>>> Clearly, CoreHandleProtocol() is a super-robust,
>> super-
>>>> mature function,
>>>> so we don't really care for its development history
>>>> today.
>>>>
>>>> I believe the same would not apply to the "edk2-
>> library"
>>>> (or
>>>> "edk2-tools") project. When it replaced the in-line
>>>> component FooBar of
>>>> BaseTools, it's plausible it would be suddenly exposed
>> to
>>>> a larger
>>>> userbase. New issues would be encountered, and people
>>>> might want to look
>>>> at the git history -- if for nothing else, then
>> rationale
>>>> (commit
>>>> messages) on small parts of the code.
>>>>
>>>>
>>>> I can also name more recent examples. I'm not a
>> frequent
>>>> BaseTools
>>>> contributor, but it has happened occasionally. (I've
>> got
>>>> 41 patches in
>>>> the edk2 git history that touched BaseTools/, as of
>>>> commit
>>>> 0a506fc7ab8b.)
>>>>
>>>> * For
>>>> <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>,
>> I
>>>> pushed
>>>>   5+1 patches:
>>>>
>>>>     67983484a443 BaseTools/footer.makefile: expand
>>>> BUILD_CFLAGS last for C files too
>>>>     03252ae287c4 BaseTools/header.makefile: remove "-
>> c"
>>>> from BUILD_CFLAGS
>>>>     b8a661702643 BaseTools/Source/C: split "-O2" to
>>>> BUILD_OPTFLAGS
>>>>     b0ca5dae78ff BaseTools/Source/C: take
>> EXTRA_OPTFLAGS
>>>> from the caller
>>>>     81502cee20ac BaseTools/Source/C: take
>> EXTRA_LDFLAGS
>>>> from the caller
>>>>     +
>>>>     aa4e0df1f0c7 BaseTools/VfrCompile: honor
>>>> EXTRA_LDFLAGS
>>>>
>>>> * For
>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>,
>> I
>>>> pushed 26
>>>>   patches:
>>>>
>>>>     8ff122119941 EmulatorPkg: require GCC48 or later
>>>>     8d7cdfae8cb8 OvmfPkg: require GCC48 or later
>>>>     fd158437dcce Vlv2TbltDevicePkg: assume GCC48 or
>> later
>>>>     7a9dbf2c94d1 BaseTools/Conf/tools_def.template:
>> drop
>>>> ARM/AARCH support from GCC46/GCC47
>>>>     48e64498c961 BaseTools/tools_def.template: fix up
>> LF-
>>>> only line terminator
>>>>     7381a6627a66 BaseTools/tools_def.template: strip
>>>> trailing whitespace
>>>>     9bbf156faaad BaseTools/tools_def.template: remove
>>>> GCC48_IA32_X64_DLINK_COMMON dead-end
>>>>     3c5613c5932c BaseTools/tools_def.template: remove
>>>> GCC47 leaf definitions
>>>>     fc87b8d7f411 BaseTools/tools_def.template:
>> propagate
>>>> loss of GCC47 references
>>>>     91a67e0f111e BaseTools/tools_def.template: remove
>>>> GCC47 documentation
>>>>     0f234fb8a662 BaseTools/tools_def.template: remove
>>>> GCC46 leaf definitions
>>>>     83a8f313884a BaseTools/tools_def.template:
>> propagate
>>>> loss of GCC46 references
>>>>     be359fa7ceec BaseTools/tools_def.template: remove
>>>> GCC46 documentation
>>>>     1458af0cbce0 BaseTools/tools_def.template: remove
>>>> GCC45 leaf definitions
>>>>     024576896d42 BaseTools/tools_def.template:
>> propagate
>>>> loss of GCC45 references
>>>>     3e77d20f5cb3 BaseTools/tools_def.template: remove
>>>> GCC45 documentation
>>>>     e046dc60fb89 BaseTools/tools_def.template: remove
>>>> GCC44 leaf definitions
>>>>     38c570efede0 BaseTools/tools_def.template:
>> propagate
>>>> loss of GCC44 references
>>>>     383d29096846 BaseTools/tools_def.template: rename
>>>> GCC44_ALL_CC_FLAGS to GCC48_ALL_CC_FLAGS
>>>>     0db91daf5228 BaseTools/tools_def.template:
>> eliminate
>>>> GCC44_IA32_X64_DLINK_FLAGS
>>>>     84d21abf4e36 BaseTools/tools_def.template: rename
>>>> GCC44_IA32_X64_DLINK_COMMON to
>>>> GCC48_IA32_X64_DLINK_COMMON
>>>>     5c6ccd53244b BaseTools/tools_def.template: remove
>>>> comment about GCC44 + LzmaF86Compress
>>>>     3bc65326d6ed BaseTools/tools_def.template: remove
>>>> GCC44 documentation
>>>>     f7282023e758 ArmPkg/ArmSoftFloatLib: drop build
>> flags
>>>> specific to GCC46/GCC47
>>>>     300b8c5f150c CryptoPkg/BaseCryptLib: drop build
>> flags
>>>> specific to GCC44
>>>>     7423ba9d499b Revert "MdePkg: avoid
>>>> __builtin_unreachable() on GCC v4.4"
>>>>
>>>>   (Note, from this list, 7a9dbf2c94d1 was authored by
>>>> Ard, I just
>>>>   included it at the right spot.)
>>>>
>>>> All of these patch series were carefully structured,
>> and
>>>> carefully
>>>> documented (in the commit messages). I would have been
>>>> devastated, had
>>>> they been squashed.
>>>>
>>>> (And it would have been questionable to squash Ard's
>>>> patch with patches
>>>> authored by me.)
>>>>
>>>>
>>>>> Is there a way for GitHub to support both squash
>>>> commits for some PRs
>>>>> and preserve a patch series for other PRs?
>>>>
>>>> I don't know.
>>>>
>>>> But honestly, what is the purpose of squash merges?
>> Don't
>>>> we expect
>>>> *all* contributions in well structured patch series?
>>>>
>>>> If review discovers an issue with the structure of the
>>>> series (for
>>>> example, not buildable in the middle, or the steps are
>>>> not logical in
>>>> that order, or some patches do too many things at once
>>>> and should be
>>>> split up, or a patch in the middle has a typo), the
>> fixes
>>>> shouldn't be
>>>> just heaped on top. The submitter should restructure
>>>> (rebase) the
>>>> series, and submit the reworked series on a new topic
>>>> branch / pull
>>>> request.
>>>>
>>>> Isn't this the contribution pattern we'd like to see
>> in
>>>> all
>>>> sub-projects, where we (the TianoCore community) have
>>>> reviewer /
>>>> maintainer responsibilities?
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>>> Thanks,
>>>>>
>>>>> Mike
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: devel@edk2.groups.io
>>>> [mailto:devel@edk2.groups.io]
>>>>>> On Behalf Of Laszlo Ersek
>>>>>> Sent: Wednesday, May 8, 2019 2:56 AM
>>>>>> To: devel@edk2.groups.io; sean.brogan@microsoft.com
>>>>>> Subject: Re: [edk2-devel] RFC for Edk2-Library
>>>>>>
>>>>>> On 05/07/19 23:35, Sean via Groups.Io wrote:
>>>>>>> RFC  Edk2-Library creation
>>>>>>>
>>>>>>> Create a new tianocore owned repository to host a
>>>>>> python library
>>>>>>> package in support of UEFI development.  This
>> package
>>>>>> will allow easy
>>>>>>> sharing of python library code to facilitate reuse.
>>>>>> Inclusion of this
>>>>>>> package and dependency management should be managed
>>>>>> using Pip/Pypi. To
>>>>>>> start this is a supplemental package and is not
>>>>>> required to be used
>>>>>>> for edk2 builds.
>>>>>>
>>>>>> [1]
>>>>>>
>>>>>>> Examples of content here
>>>>>>>
>>>>>>> * Edk2 file type parsing
>>>>>>> * UEFI structure encode/decode in python
>>>>>>> * Packaging tools (Capsules generation, signing,
>> INF
>>>>>> gen, Cat gen)
>>>>>>> * TPM support code
>>>>>>> * Potentially move content from
>>>>>> basetools/source/python/common/*
>>>>>>
>>>>>> [2]
>>>>>>
>>>>>>> * No command line tools/interface
>>>>>>>
>>>>>>> Maintainers
>>>>>>>
>>>>>>> * Sean Brogan
>>>>>>> * Bret Barkelew
>>>>>>> * Placeholder for existing maintainer from the
>>>>>> basetools
>>>>>>>
>>>>>>> License
>>>>>>>
>>>>>>> * BSD + Patent (edk2 aligned)
>>>>>>>
>>>>>>> Contribution process and issue tracking
>>>>>>>
>>>>>>> * Follow Github PR process for contributions and
>>>> issue
>>>>>> tracking
>>>>>>> * Contributor forks repo in github
>>>>>>> * Contributor creates branch for work
>>>>>>> * Contributor updates release notes to indicate
>>>> change
>>>>>> (if necessary)
>>>>>>> * Contributor submits PR to master branch of
>>>>>> tianocore/Edk2-Library
>>>>>>>   repo
>>>>>>> * Review feedback is given in PR
>>>>>>> * Python Tests are run on the repo (new
>> contributions
>>>>>> need unit tests)
>>>>>>> * Python Style (flake8) must pass
>>>>>>> * All review feedback must be completed,
>> maintainers
>>>>>> approved, and
>>>>>>>   tests run successfully before PR is *squash
>> merged*
>>>>>> into master
>>>>>>
>>>>>> The sentences
>>>>>>
>>>>>> [1] "To start this is a supplemental package and is
>>>> not
>>>>>> required to be
>>>>>>      used for edk2 builds."
>>>>>>
>>>>>> [2] "Potentially move content from
>>>>>> basetools/source/python/common/*"
>>>>>>
>>>>>> foreshadow that such a code movement might happen
>> down
>>>>>> the road, and the
>>>>>> external package could become a requirement for
>>>> building
>>>>>> edk2.
>>>>>>
>>>>>> That step would mean the following:
>>>>>>
>>>>>> (a) Edk2 would not remain buildable from a single
>>>>>> command
>>>>>>
>>>>>>     git clone --recurse-submodules
>>>>>>
>>>>>>     Building edk2 would require GNU/Linux users to
>>>> start
>>>>>> tracking
>>>>>>     packages with "pip", which is independent of any
>>>>>> given distro's own
>>>>>>     package management, and may cause conflicts if
>> not
>>>>>> used carefully:
>>>>>>
>>>>>>
>>>>>>
>>>>
>> https://developer.fedoraproject.org/tech/languages/pytho
>>>>>> n/pypi-installation.html
>>>>>>
>>>>>>     This requirement on "pip" would only go away
>> once
>>>>>> the external
>>>>>>     python dependencies were packaged for at least
>> the
>>>>>> larger GNU/Linux
>>>>>>     distros.
>>>>>>
>>>>>> (b) Edk2 users running into build problems related
>> to
>>>>>> the external
>>>>>>     python dependencies would have to contribute
>>>> through
>>>>>> a github-only
>>>>>>     workflow. That's not a deal-breaker per se -- if
>>>> we
>>>>>> want to
>>>>>>     contribute to other edk2 dependencies, such as
>>>>>> OpenSSL or nasm, we
>>>>>>     also have to conform to their specific
>> development
>>>>>> models, clearly.
>>>>>>
>>>>>>     However, "squash merge" is a catastrophically
>> bad
>>>>>> development model,
>>>>>>     and I'd object to introducing a new edk2 build
>>>>>> dependency that
>>>>>>     followed that model.
>>>>>>
>>>>>>     (There are other issues with the GitHub.com
>>>>>> development workflow, as
>>>>>>     discussed elsewhere, but "squash merge" takes
>> the
>>>>>> cake.)
>>>>>>
>>>>>> Problem (a) would be solvable in the long term
>>>> (through
>>>>>> collaboration
>>>>>> with distro maintainers, i.e. downstream packaging),
>>>> but
>>>>>> problem (b)
>>>>>> would not be. Thus I'm fine with the proposal, in
>> its
>>>>>> current form, only
>>>>>> if the new package is 100% an addition on top of
>> edk2,
>>>>>> even in the long
>>>>>> term, and not in any part intended as a future
>>>>>> replacement for current
>>>>>> edk2 functionality.
>>>>>>
>>>>>> Thanks,
>>>>>> Laszlo
>>>>>>
>>>>>>
>>>>>
>>>
>>
>>
>> 
> 

Re: [edk2-devel] RFC for Edk2-Library

[Michael D Kinney]

Hi Sean,

Can you send an RFC V2 that makes the following changes:

1) Add use of python virtual environments
2) Update Repository name: edk2-tools-library
3) Provide a summary of the APIs/Services that this PIP module 
   provides and the APIs/Services from the edk2 repo that this
   PIP module depends on.
4) Contribution process.  Add recommendation that PRs be focused
   on changes that make sense to be squashed.  Submit a different
   PR for a different feature/issue.  Break up a complex PR into
   multiple PRs.
5) Remove the following bullet:

   "* Potentially move content from basetools/source/python/common/*"

   We can discuss this concept after TianoCore platforms are
   successfully using these new features and the overlap between
   the edk2 repo and edk2-tools-library repo is clearly understood.

A follow on task should evaluate GitHub PR options for submitting
and preserving a patch series.

Thanks,

Mike


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, May 13, 2019 1:29 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; sean.brogan@microsoft.com
> Subject: Re: [edk2-devel] RFC for Edk2-Library
> 
> On 05/13/19 20:20, Kinney, Michael D wrote:
> > Laszlo,
> >
> > On Windows build systems, we have to install OpenSSL
> command line
> > utilities.  For all host systems, the OpenSSL command
> line
> > utilities need to be in the system path.  My point is
> that this
> > is similar to other dependencies like iASL and NASM.
> 
> OK. I think I must have misunderstood you at some point.
> Sorry about that.
> 
> > For the patch discussion, I did not mean to confuse
> things.  From
> > one perspective, there are two types of patch
> submissions.  Single
> > commit (no series) and multiple commits (patch series).
> The squash
> > merge of a PR works just fine for the single commit (no
> series) type.
> > There may be feedback/comments that require code
> changes and once the
> > PR is accepted, the history in the repository shows a
> single commit.
> > As the PR evolves, commits are made on the PR to
> address each piece
> > of feedback.  Squashing all of these together at the
> time the PR is
> > accepted is the correct action and matches what we do
> the for email
> > based review process.  The final result for both PR and
> email is a
> > single commit with a cleaned up commit message.
> 
> OK.
> 
> > You may consider the single commit (no series) type
> more rare than the
> > multiple commit (patch series) type.  However, there
> may be cases where
> > a multiple commit (patch series) was used where the
> changes could have
> > been submitted as a set of single commit (no series)
> changes.
> 
> OK.
> 
> Thanks
> Laszlo
> 
> >
> > Best regards,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Monday, May 13, 2019 3:46 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> >> devel@edk2.groups.io; sean.brogan@microsoft.com
> >> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>
> >> On 05/10/19 02:01, Kinney, Michael D wrote:
> >>> Laszlo,
> >>>
> >>> 1) We also use OpenSSL command line tool to locally
> >> sign
> >>>    capsules and recovery images for local testing.
> So
> >> both
> >>>    tool dependency and source dependency apply to the
> >> OpenSSL
> >>>    content.
> >>
> >> I haven't used the tools yet that you refer to, so I'm
> >> unsure how
> >> exactly they invoke the "openssl" utility. If they
> just
> >> rely on the PATH
> >> environment variable, then what they invoke comes from
> >> the system-wide
> >> openssl package.
> >>
> >>>
> >>> 2) If a dev submits a PR, and there are many review
> >> comments
> >>>    that require code changes, then those code changes
> >> are
> >>>    added to that PR until all feedback is addressed.
> >>
> >> I don't understand how. Let's say the pull request
> refers
> >> to a branch
> >> with three commits, and the majority of the review
> >> comments request
> >> updates for patch #2 (i.e., in the middle). How can
> the
> >> submitter "add
> >> changes to the PR"? It is patch #2 that needs to be
> >> reworked, which will
> >> require rebases, and so the hash of the HEAD commit of
> >> the topic branch
> >> (patch #3) will change as well.
> >>
> >>>    At that
> >>>    point, if the change is something that should be
> in
> >> a single
> >>>    commit, then doing a squash merge with a cleaned
> up
> >> commit
> >>>    message would be appropriate.
> >>
> >> I don't understand -- we modify only patch #2, yes, to
> >> address review
> >> comments, but why does that justify squashing #1
> through
> >> #3 into a
> >> single commit?
> >>
> >>>    And the history of all the
> >>>    review feedback preserved in the PR.
> >>
> >> That's good (but it could be better -- see the lacking
> >> email
> >> integration. Anyway this is not strictly tied to my
> >> concern with
> >> squash-on-merge).
> >>
> >>>
> >>>    If we create a 2nd PR with the cleaned up content,
> >> then the
> >>>    connection to the 1st PR feedback may be lost.  I
> >> agree this
> >>>    matches what we do in email reviews to create a V2
> >> patch series.
> >>>    The V1 and V2 threads are in the email archive.
> >>
> >> Indeed. And the blurb for both threads reference the
> >> bugzilla, and the
> >> bugzilla has (if the submitter is careful) comments
> >> linking the v1 and
> >> v2 threads from the email archive.
> >>
> >>>    I wonder if
> >>>    there is a way to link a 2nd PR to the 1st PR and
> >> guarantee
> >>>    that both PRs are preserved?  This would also
> allow
> >> the 2nd
> >>>    cleaned up PR to contain a series, and if there
> was
> >> a way to
> >>>    make the squash merge optional, then the developer
> >> can choose
> >>>    the way the patches are committed.
> >>
> >> This sounds great! I think both PRs can reference the
> >> bugzilla ticket,
> >> and the bugzilla ticket can reference both PRs too (as
> >> comments). The
> >> first PR can be rejected & closed when the second one
> is
> >> submitted.
> >>
> >>>    Just a few ideas to explore...Perhaps Sean can
> >> provide some
> >>>    additional ideas to manage complex changes in PRs.
> >>
> >> Thanks!
> >> Laszlo
> >>
> >>>
> >>> Best regards,
> >>>
> >>> Mike
> >>>
> >>>> -----Original Message-----
> >>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >>>> Sent: Thursday, May 9, 2019 3:56 PM
> >>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> >>>> devel@edk2.groups.io; sean.brogan@microsoft.com
> >>>> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>>>
> >>>> On 05/09/19 20:06, Kinney, Michael D wrote:
> >>>>> Hello,
> >>>>>
> >>>>> It is difficult to tell if the repo name edk2-
> library
> >>>> is for firmware
> >>>>> or tools, so I recommend we work on a name that
> >> clearly
> >>>> identifies
> >>>>> that this repo is related to tools.
> >>>>
> >>>> Good idea.
> >>>>
> >>>>> For the pip dependencies, is the concern that a
> >>>> platform that depends
> >>>>> on these tools will not be buildable without
> running
> >> a
> >>>> "pip install"
> >>>>> command that pulls content from the network?
> >>>>
> >>>> Almost. Not exactly.
> >>>>
> >>>> First, the concern is not specific to any given
> >> platform.
> >>>> My
> >>>> understanding is that the extraction would target,
> in
> >> the
> >>>> longer term,
> >>>> general BaseTools functionality. That would impact
> all
> >>>> platforms that
> >>>> consume & build against edk2.
> >>>>
> >>>> Second, it's not the network access that's
> concerning
> >> per
> >>>> se. It's the
> >>>> compatibility / interoperation with any given Linux
> >>>> distribution's own
> >>>> (native) package management system. Such a package
> >>>> management system is
> >>>> generally not used on Windows, therefore Windows
> users
> >>>> are accustomed to
> >>>> installing software packages from a boatload of
> >> vendors.
> >>>> This is not the
> >>>> case on Linux distros -- your distro vendor offers a
> >>>> curated set of
> >>>> packages, such that everything interoperates with
> >>>> everything (ideally),
> >>>> there are no file conflicts, version dependencies
> are
> >>>> handled
> >>>> automatically (e.g. if you install a high-level
> >> package,
> >>>> its
> >>>> dependencies are pulled in automatically), and so
> on.
> >>>>
> >>>> Clearly the distro vendor does not *author* all this
> >>>> software (although
> >>>> they are strongly encouraged to contribute to the
> >>>> upstream software
> >>>> projects they package and ship). The distro vendor
> is
> >>>> responsible for
> >>>> the integration of all these packages into a
> >> consistent
> >>>> OS, into
> >>>> consistent feature sets, and so on.
> >>>>
> >>>> "pip" and similar tools are generally unfit for this
> >>>> approach, because
> >>>> they implement a parallel package management system,
> >> to
> >>>> my
> >>>> understanding. If they are carefully used in a
> user's
> >>>> home directory,
> >>>> they might work. If packages were installed with
> "pip"
> >>>> system-wide, they
> >>>> would almost certainly break (conflict with) the
> >> distro-
> >>>> provided
> >>>> packages.
> >>>>
> >>>> Therefore, when users would like to get a new piece
> of
> >>>> software
> >>>> packaged, or an existent package refreshed (to a
> more
> >>>> recent upstream
> >>>> version), they file a feature requst with their
> distro
> >>>> vendor (unless
> >>>> the distro vendor already tracks the upstream
> project
> >>>> closely and
> >>>> performs periodic rebases anyway). Then the distro
> >> vendor
> >>>> ships a new
> >>>> (or refreshed) package, again nicely integrated with
> >> the
> >>>> rest of the
> >>>> system.
> >>>>
> >>>>> We already have to pull content to get the sources
> >> and
> >>>> potentially
> >>>>> other dependent tools (NASM, iASL, openssl).
> >>>>
> >>>> Yes, these are good examples to demonstrate the
> >>>> difference. Consider
> >>>> NASM. If a Windows user would like to install NASM,
> >> they
> >>>> google "NASM
> >>>> for Windows", then go to:
> >>>>
> >>>>
> >>
> https://www.nasm.us/pub/nasm/releasebuilds/2.14.02/win64/
> >>>>
> >>>> download the ZIP file or EXE file, and install it.
> >>>>
> >>>> In comparison, a Fedora user runs
> >>>>
> >>>>   dnf install nasm
> >>>>
> >>>> and they get the package from the Fedora package
> >>>> repository. A Debian
> >>>> user would run
> >>>>
> >>>>   apt-get install nasm
> >>>>
> >>>> and they would get the package (which would be
> >> entirely
> >>>> independent of
> >>>> Fedora's) from the Debian package repository.
> >>>>
> >>>> The various *BSDs would run a variant of
> >>>>
> >>>>   pkg_add nasm
> >>>>
> >>>> I believe.
> >>>>
> >>>> The same applies to iASL.
> >>>>
> >>>>
> >>>> OpenSSL is a special case. It is different from NASM
> >> and
> >>>> iASL. NASM and
> >>>> iASL are native (hosted) applications, so any Linux
> >>>> distro can easily
> >>>> build them (for itself), and the binaries can be
> >> shipped
> >>>> to users, ready
> >>>> to run. But for the purposes of edk2, OpenSSL is not
> >>>> needed as a native
> >>>> (hosted) utility, like NASM and iASL -- it is needed
> >> as a
> >>>> *cross-built*
> >>>> static library, targeting the firmware architecture
> >> (not
> >>>> the host
> >>>> architecture). Unfortunately, OpenSSL (the upstream
> >>>> project) does not
> >>>> fully support this use case yet, therefore Linux
> >> distros
> >>>> cannot just
> >>>> cross-compile OpenSSL for edk2, and offer static
> >> library
> >>>> packages. This
> >>>> is why upstream edk2 consumes OpenSSL through a git
> >>>> submodule, in source
> >>>> form.
> >>>>
> >>>> *However*: conceptually, there is no difference. In
> >>>> Fedora and RHEL, we
> >>>> don't build OVMF against upstream OpenSSL. We build
> it
> >>>> against the
> >>>> downstream OpenSSL source code. So ultimately there
> is
> >> no
> >>>> difference in
> >>>> the distro vendor's approach -- the sources and the
> >>>> binaries come from
> >>>> the distro vendor.
> >>>>
> >>>>
> >>>> So what follows from this? It means that, for using
> >>>> BaseTools on a Linux
> >>>> distro, users will have to install some extra Python
> >>>> packages, in
> >>>> addition to NASM and iASL, from their vendor's
> >>>> repository. In other
> >>>> words, the "edk2-library" (or "edk2-tools") project
> >>>> should do upstream
> >>>> releases, so that Linux distros can package them in
> >> their
> >>>> native package
> >>>> systems. And some collaboration between upstream and
> >>>> downstreams is
> >>>> expected for this. (Exactly as it occurs with the
> iASL
> >>>> and NASM upstream
> >>>> projects.)
> >>>>
> >>>>> Can we limit the initial scope to tools that layer
> on
> >>>> top of edk2, and
> >>>>> a different future RFC would be required if there
> is
> >> a
> >>>> proposal for
> >>>>> the edk2 repo to depend on another repo?
> >>>>
> >>>> Yes, that would be very nice.
> >>>>
> >>>>> If we accept this limited scope, are there still
> >>>> concerns about
> >>>>> initially using squash commits for changes to these
> >>>> tools.
> >>>>
> >>>> Yes, I still have the same concern -- although it's
> a
> >>>> milder concern,
> >>>> dependent on how long squash merges are tolerated.
> >>>>
> >>>> Assume that a second, separate RFC gets posted down
> >> the
> >>>> road, let's say
> >>>> a year in, which proposes to replace the in-line
> >>>> component FooBar of
> >>>> BaseTools, with the external project "edk2-library"
> >> (or
> >>>> "edk2-tools").
> >>>> At that point, the git history accrued thus far, in
> >> edk2-
> >>>> tools, would
> >>>> suddenly be inherited by all BaseTools users. And
> that
> >>>> history wouldn't
> >>>> be really helpful.
> >>>>
> >>>> For an example, we need not look past the edk2
> >> repository
> >>>> itself. Let's
> >>>> say I'd like to learn about the implementation
> history
> >> of
> >>>> the
> >>>> HandleProtocol() boot service, in edk2. So I run:
> >>>>
> >>>> $ git blame master --
> >> MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>>
> >>>> and I look up the CoreHandleProtocol() function, and
> >> find
> >>>> this:
> >>>>
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  931)
> >>>> EFI_STATUS
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  932)
> >> EFIAPI
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  933)
> >>>> CoreHandleProtocol (
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  934)
> IN
> >>>> EFI_HANDLE       UserHandle,
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  935)
> IN
> >>>> EFI_GUID         *Protocol,
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  936)
> >> OUT
> >>>> VOID            **Interface
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  937)
> )
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  938) {
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  939)
> >>>> return CoreOpenProtocol (
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  940)
> >>>> UserHandle,
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  941)
> >>>> Protocol,
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  942)
> >>>> Interface,
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  943)
> >>>> gDxeCoreImageHandle,
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  944)
> >>>> NULL,
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  945)
> >>>> EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  946)
> >>>> );
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  947) }
> >>>>
> >>>> Interesting, it seems like the function was
> originally
> >>>> added in commit
> >>>> 28a00297189c3, and then the argument list was
> modified
> >> in
> >>>> commit
> >>>> 022c6d45ef786, a year later. So let's check out the
> >>>> rationale for that
> >>>> argument list update:
> >>>>
> >>>> $ git show --color 022c6d45ef786
> >>>>
> >>>> Well... The commit message says, "Code Scrub for Dxe
> >>>> Core". That's all.
> >>>> We also learn it comes from SVN revision 5560. And
> the
> >>>> diffstat for the
> >>>> patch is:
> >>>>
> >>>>>  38 files changed, 2499 insertions(+), 2504
> >> deletions(-
> >>>> )
> >>>>
> >>>> It is not overly helpful.
> >>>>
> >>>> Clearly, CoreHandleProtocol() is a super-robust,
> >> super-
> >>>> mature function,
> >>>> so we don't really care for its development history
> >>>> today.
> >>>>
> >>>> I believe the same would not apply to the "edk2-
> >> library"
> >>>> (or
> >>>> "edk2-tools") project. When it replaced the in-line
> >>>> component FooBar of
> >>>> BaseTools, it's plausible it would be suddenly
> exposed
> >> to
> >>>> a larger
> >>>> userbase. New issues would be encountered, and
> people
> >>>> might want to look
> >>>> at the git history -- if for nothing else, then
> >> rationale
> >>>> (commit
> >>>> messages) on small parts of the code.
> >>>>
> >>>>
> >>>> I can also name more recent examples. I'm not a
> >> frequent
> >>>> BaseTools
> >>>> contributor, but it has happened occasionally. (I've
> >> got
> >>>> 41 patches in
> >>>> the edk2 git history that touched BaseTools/, as of
> >>>> commit
> >>>> 0a506fc7ab8b.)
> >>>>
> >>>> * For
> >>>>
> <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>,
> >> I
> >>>> pushed
> >>>>   5+1 patches:
> >>>>
> >>>>     67983484a443 BaseTools/footer.makefile: expand
> >>>> BUILD_CFLAGS last for C files too
> >>>>     03252ae287c4 BaseTools/header.makefile: remove
> "-
> >> c"
> >>>> from BUILD_CFLAGS
> >>>>     b8a661702643 BaseTools/Source/C: split "-O2" to
> >>>> BUILD_OPTFLAGS
> >>>>     b0ca5dae78ff BaseTools/Source/C: take
> >> EXTRA_OPTFLAGS
> >>>> from the caller
> >>>>     81502cee20ac BaseTools/Source/C: take
> >> EXTRA_LDFLAGS
> >>>> from the caller
> >>>>     +
> >>>>     aa4e0df1f0c7 BaseTools/VfrCompile: honor
> >>>> EXTRA_LDFLAGS
> >>>>
> >>>> * For
> >>>>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>,
> >> I
> >>>> pushed 26
> >>>>   patches:
> >>>>
> >>>>     8ff122119941 EmulatorPkg: require GCC48 or later
> >>>>     8d7cdfae8cb8 OvmfPkg: require GCC48 or later
> >>>>     fd158437dcce Vlv2TbltDevicePkg: assume GCC48 or
> >> later
> >>>>     7a9dbf2c94d1 BaseTools/Conf/tools_def.template:
> >> drop
> >>>> ARM/AARCH support from GCC46/GCC47
> >>>>     48e64498c961 BaseTools/tools_def.template: fix
> up
> >> LF-
> >>>> only line terminator
> >>>>     7381a6627a66 BaseTools/tools_def.template: strip
> >>>> trailing whitespace
> >>>>     9bbf156faaad BaseTools/tools_def.template:
> remove
> >>>> GCC48_IA32_X64_DLINK_COMMON dead-end
> >>>>     3c5613c5932c BaseTools/tools_def.template:
> remove
> >>>> GCC47 leaf definitions
> >>>>     fc87b8d7f411 BaseTools/tools_def.template:
> >> propagate
> >>>> loss of GCC47 references
> >>>>     91a67e0f111e BaseTools/tools_def.template:
> remove
> >>>> GCC47 documentation
> >>>>     0f234fb8a662 BaseTools/tools_def.template:
> remove
> >>>> GCC46 leaf definitions
> >>>>     83a8f313884a BaseTools/tools_def.template:
> >> propagate
> >>>> loss of GCC46 references
> >>>>     be359fa7ceec BaseTools/tools_def.template:
> remove
> >>>> GCC46 documentation
> >>>>     1458af0cbce0 BaseTools/tools_def.template:
> remove
> >>>> GCC45 leaf definitions
> >>>>     024576896d42 BaseTools/tools_def.template:
> >> propagate
> >>>> loss of GCC45 references
> >>>>     3e77d20f5cb3 BaseTools/tools_def.template:
> remove
> >>>> GCC45 documentation
> >>>>     e046dc60fb89 BaseTools/tools_def.template:
> remove
> >>>> GCC44 leaf definitions
> >>>>     38c570efede0 BaseTools/tools_def.template:
> >> propagate
> >>>> loss of GCC44 references
> >>>>     383d29096846 BaseTools/tools_def.template:
> rename
> >>>> GCC44_ALL_CC_FLAGS to GCC48_ALL_CC_FLAGS
> >>>>     0db91daf5228 BaseTools/tools_def.template:
> >> eliminate
> >>>> GCC44_IA32_X64_DLINK_FLAGS
> >>>>     84d21abf4e36 BaseTools/tools_def.template:
> rename
> >>>> GCC44_IA32_X64_DLINK_COMMON to
> >>>> GCC48_IA32_X64_DLINK_COMMON
> >>>>     5c6ccd53244b BaseTools/tools_def.template:
> remove
> >>>> comment about GCC44 + LzmaF86Compress
> >>>>     3bc65326d6ed BaseTools/tools_def.template:
> remove
> >>>> GCC44 documentation
> >>>>     f7282023e758 ArmPkg/ArmSoftFloatLib: drop build
> >> flags
> >>>> specific to GCC46/GCC47
> >>>>     300b8c5f150c CryptoPkg/BaseCryptLib: drop build
> >> flags
> >>>> specific to GCC44
> >>>>     7423ba9d499b Revert "MdePkg: avoid
> >>>> __builtin_unreachable() on GCC v4.4"
> >>>>
> >>>>   (Note, from this list, 7a9dbf2c94d1 was authored
> by
> >>>> Ard, I just
> >>>>   included it at the right spot.)
> >>>>
> >>>> All of these patch series were carefully structured,
> >> and
> >>>> carefully
> >>>> documented (in the commit messages). I would have
> been
> >>>> devastated, had
> >>>> they been squashed.
> >>>>
> >>>> (And it would have been questionable to squash Ard's
> >>>> patch with patches
> >>>> authored by me.)
> >>>>
> >>>>
> >>>>> Is there a way for GitHub to support both squash
> >>>> commits for some PRs
> >>>>> and preserve a patch series for other PRs?
> >>>>
> >>>> I don't know.
> >>>>
> >>>> But honestly, what is the purpose of squash merges?
> >> Don't
> >>>> we expect
> >>>> *all* contributions in well structured patch series?
> >>>>
> >>>> If review discovers an issue with the structure of
> the
> >>>> series (for
> >>>> example, not buildable in the middle, or the steps
> are
> >>>> not logical in
> >>>> that order, or some patches do too many things at
> once
> >>>> and should be
> >>>> split up, or a patch in the middle has a typo), the
> >> fixes
> >>>> shouldn't be
> >>>> just heaped on top. The submitter should restructure
> >>>> (rebase) the
> >>>> series, and submit the reworked series on a new
> topic
> >>>> branch / pull
> >>>> request.
> >>>>
> >>>> Isn't this the contribution pattern we'd like to see
> >> in
> >>>> all
> >>>> sub-projects, where we (the TianoCore community)
> have
> >>>> reviewer /
> >>>> maintainer responsibilities?
> >>>>
> >>>> Thanks,
> >>>> Laszlo
> >>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Mike
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: devel@edk2.groups.io
> >>>> [mailto:devel@edk2.groups.io]
> >>>>>> On Behalf Of Laszlo Ersek
> >>>>>> Sent: Wednesday, May 8, 2019 2:56 AM
> >>>>>> To: devel@edk2.groups.io;
> sean.brogan@microsoft.com
> >>>>>> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>>>>>
> >>>>>> On 05/07/19 23:35, Sean via Groups.Io wrote:
> >>>>>>> RFC  Edk2-Library creation
> >>>>>>>
> >>>>>>> Create a new tianocore owned repository to host a
> >>>>>> python library
> >>>>>>> package in support of UEFI development.  This
> >> package
> >>>>>> will allow easy
> >>>>>>> sharing of python library code to facilitate
> reuse.
> >>>>>> Inclusion of this
> >>>>>>> package and dependency management should be
> managed
> >>>>>> using Pip/Pypi. To
> >>>>>>> start this is a supplemental package and is not
> >>>>>> required to be used
> >>>>>>> for edk2 builds.
> >>>>>>
> >>>>>> [1]
> >>>>>>
> >>>>>>> Examples of content here
> >>>>>>>
> >>>>>>> * Edk2 file type parsing
> >>>>>>> * UEFI structure encode/decode in python
> >>>>>>> * Packaging tools (Capsules generation, signing,
> >> INF
> >>>>>> gen, Cat gen)
> >>>>>>> * TPM support code
> >>>>>>> * Potentially move content from
> >>>>>> basetools/source/python/common/*
> >>>>>>
> >>>>>> [2]
> >>>>>>
> >>>>>>> * No command line tools/interface
> >>>>>>>
> >>>>>>> Maintainers
> >>>>>>>
> >>>>>>> * Sean Brogan
> >>>>>>> * Bret Barkelew
> >>>>>>> * Placeholder for existing maintainer from the
> >>>>>> basetools
> >>>>>>>
> >>>>>>> License
> >>>>>>>
> >>>>>>> * BSD + Patent (edk2 aligned)
> >>>>>>>
> >>>>>>> Contribution process and issue tracking
> >>>>>>>
> >>>>>>> * Follow Github PR process for contributions and
> >>>> issue
> >>>>>> tracking
> >>>>>>> * Contributor forks repo in github
> >>>>>>> * Contributor creates branch for work
> >>>>>>> * Contributor updates release notes to indicate
> >>>> change
> >>>>>> (if necessary)
> >>>>>>> * Contributor submits PR to master branch of
> >>>>>> tianocore/Edk2-Library
> >>>>>>>   repo
> >>>>>>> * Review feedback is given in PR
> >>>>>>> * Python Tests are run on the repo (new
> >> contributions
> >>>>>> need unit tests)
> >>>>>>> * Python Style (flake8) must pass
> >>>>>>> * All review feedback must be completed,
> >> maintainers
> >>>>>> approved, and
> >>>>>>>   tests run successfully before PR is *squash
> >> merged*
> >>>>>> into master
> >>>>>>
> >>>>>> The sentences
> >>>>>>
> >>>>>> [1] "To start this is a supplemental package and
> is
> >>>> not
> >>>>>> required to be
> >>>>>>      used for edk2 builds."
> >>>>>>
> >>>>>> [2] "Potentially move content from
> >>>>>> basetools/source/python/common/*"
> >>>>>>
> >>>>>> foreshadow that such a code movement might happen
> >> down
> >>>>>> the road, and the
> >>>>>> external package could become a requirement for
> >>>> building
> >>>>>> edk2.
> >>>>>>
> >>>>>> That step would mean the following:
> >>>>>>
> >>>>>> (a) Edk2 would not remain buildable from a single
> >>>>>> command
> >>>>>>
> >>>>>>     git clone --recurse-submodules
> >>>>>>
> >>>>>>     Building edk2 would require GNU/Linux users to
> >>>> start
> >>>>>> tracking
> >>>>>>     packages with "pip", which is independent of
> any
> >>>>>> given distro's own
> >>>>>>     package management, and may cause conflicts if
> >> not
> >>>>>> used carefully:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>
> https://developer.fedoraproject.org/tech/languages/pytho
> >>>>>> n/pypi-installation.html
> >>>>>>
> >>>>>>     This requirement on "pip" would only go away
> >> once
> >>>>>> the external
> >>>>>>     python dependencies were packaged for at least
> >> the
> >>>>>> larger GNU/Linux
> >>>>>>     distros.
> >>>>>>
> >>>>>> (b) Edk2 users running into build problems related
> >> to
> >>>>>> the external
> >>>>>>     python dependencies would have to contribute
> >>>> through
> >>>>>> a github-only
> >>>>>>     workflow. That's not a deal-breaker per se --
> if
> >>>> we
> >>>>>> want to
> >>>>>>     contribute to other edk2 dependencies, such as
> >>>>>> OpenSSL or nasm, we
> >>>>>>     also have to conform to their specific
> >> development
> >>>>>> models, clearly.
> >>>>>>
> >>>>>>     However, "squash merge" is a catastrophically
> >> bad
> >>>>>> development model,
> >>>>>>     and I'd object to introducing a new edk2 build
> >>>>>> dependency that
> >>>>>>     followed that model.
> >>>>>>
> >>>>>>     (There are other issues with the GitHub.com
> >>>>>> development workflow, as
> >>>>>>     discussed elsewhere, but "squash merge" takes
> >> the
> >>>>>> cake.)
> >>>>>>
> >>>>>> Problem (a) would be solvable in the long term
> >>>> (through
> >>>>>> collaboration
> >>>>>> with distro maintainers, i.e. downstream
> packaging),
> >>>> but
> >>>>>> problem (b)
> >>>>>> would not be. Thus I'm fine with the proposal, in
> >> its
> >>>>>> current form, only
> >>>>>> if the new package is 100% an addition on top of
> >> edk2,
> >>>>>> even in the long
> >>>>>> term, and not in any part intended as a future
> >>>>>> replacement for current
> >>>>>> edk2 functionality.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Laszlo
> >>>>>>
> >>>>>>
> >>>>>
> >>>
> >>
> >>
> >> 
> >

[edk2-devel] RFC for edk2-tools-library v2 previously known as RFC for Edk2-Library

[Sean]

[-- Attachment #1: Type: text/plain, Size: 5564 bytes --]

RFC  edk2-tools-library creation

Create a new tianocore owned repository to host a python library package in support of UEFI development.  This package will allow easy sharing of python library code to facilitate reuse.  Inclusion of this package and dependency management should be managed using Pip/Pypi.   This is a supplemental package and is not required to be used for edk2 builds. To avoid conflicting dependencies on your host platform it is strongly suggested to leverage python virtual environments.

Examples of content here

* Edk2 file type parsing
* UEFI structure encode/decode in python
* Packaging tools (Capsules generation, signing, INF gen, Cat gen)
* TPM support code
* No command line tools/interface

Maintainers

* Sean Brogan
* Bret Barkelew
* Placeholder for existing maintainer from the basetools

License

* BSD + Patent (edk2 aligned)

Contribution process and issue tracking

* Follow Github PR process for contributions and issue tracking
* Contributor forks repo in github
* Contributor creates branch for work
* Contributor updates release notes to indicate change (if necessary)
* Contributor submits PR to master branch of tianocore/edk2-tools-library repo
* Review feedback is given in PR
* Python Tests are run on the repo (new contributions need unit tests)
* Python Style (flake8) must pass
* All review feedback must be completed, maintainers approved, and tests run successfully before PR is squash merged into master. Since squash merge is used there is no support for patchsets. The PR should be sized appropriately.

Documentation

* Use Github IO documentation/wiki hosting
* Example content
* https://microsoft.github.io/mu/dyn/mu_pip_python_library/developing/ ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmicrosoft.github.io%2Fmu%2Fdyn%2Fmu_pip_python_library%2Fdeveloping%2F&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=fKIb1Pfj4AqoGVOWudcyFMxMypJk%2FHTts9aMxZ8HukI%3D&reserved=0 )
* https://microsoft.github.io/mu/dyn/mu_pip_python_library/publishing/ ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmicrosoft.github.io%2Fmu%2Fdyn%2Fmu_pip_python_library%2Fpublishing%2F&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=QJMUMB1hIusaRVJhgsi9kF9KIbgdhS0WRnIXVkGeBCM%3D&reserved=0 )
* Readme at root of repo
* Example: https://github.com/Microsoft/mu_pip_python_library ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Fmu_pip_python_library&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=DiuPnNMagvYYf0XXxDycSVHqijBBcDT0fXHzVY9U6%2Fw%3D&reserved=0 )
* API documentation
* Figure out autogenerated pydocs

CI Builds

* CI build process using dev ops (new TianoCore organization created in Azure devops)
* Validation is done thru build process
* Release publication done thru manual CI Build
* Examples from Mu-Python-Library
* Windows CI - https://dev.azure.com/projectmu/mu%20pip/_build?definitionId=13 ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Fprojectmu%2Fmu%2520pip%2F_build%3FdefinitionId%3D13&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=jQCCZo2u8JVisCchOoMLSgKJbG3YEk%2FG1JP9fI4g2JY%3D&reserved=0 )
* Linux CI - https://dev.azure.com/projectmu/mu%20pip/_build?definitionId=12 ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Fprojectmu%2Fmu%2520pip%2F_build%3FdefinitionId%3D12&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=CILlTdeEGpsi%2BCQNiZdIqd2Vt2RQV6L3qjz2rWEARYE%3D&reserved=0 )
* Publishing - https://dev.azure.com/projectmu/mu%20pip/_build?definitionId=16 ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Fprojectmu%2Fmu%2520pip%2F_build%3FdefinitionId%3D16&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=hkpzn9P6RBjduYBWjN56Y2qRBWyOG32mFkP%2BwtY7KBQ%3D&reserved=0 )

Release

* Release to Pypi as edk2-tools-library for easy usage in product environment
* Versioned follows: Aa.bb.cc and is based on tags in git
* AA == Major version.  Changes don’t need to be backward compatible
* BB == Minor version.  Significant new features.  Backward compatibility maintained
* CC == Bug fix/patch/small optional feature
* Package on Pypi will be owned by Tianocore group
* Example for mu-python-library: https://pypi.org/project/mu-python-library/ ( https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpypi.org%2Fproject%2Fmu-python-library%2F&data=01%7C01%7Csean.brogan%40microsoft.com%7C47c4ca03e19b4fffc8ad08d6d314774a%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=5nTb93dDnAo%2FsWyfv1zYEIkJF8L58YY5P3BkzKi4Ivc%3D&reserved=0 )

Other Notes

* Only support Python 3 (prefer 3.7+)
* This was discussed on the edk2 design meetings (4/18) https://edk2.groups.io/g/devel/files/Designs/2019/0418 ( https://edk2.groups.io/g/devel/files/Designs/2019/0418 )
* There will be one more RFC for another python package/repo for edk2-tools-extensions
* Will start version 00.09.00 while getting the repository setup and functioning with 1.0.0 being the first real release

[-- Attachment #2: Type: text/html, Size: 20406 bytes --]

Re: [edk2-devel] RFC for edk2-tools-library v2 previously known as RFC for Edk2-Library

[Sean]

[-- Attachment #1: Type: text/plain, Size: 99 bytes --]

One final update as requested.   Changing the repo name and pip module to *edk2-pytool-library*

[-- Attachment #2: Type: text/html, Size: 179 bytes --]

Re: [edk2-devel] RFC for edk2-tools-library v2 previously known as RFC for Edk2-Library

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

Hi Sean,

You have addressed all the feedback for this RFC.

I have created the edk2-pytool-library repository.

Mike

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Sean via Groups.Io
Sent: Friday, May 31, 2019 10:40 AM
To: Sean <sean.brogan@microsoft.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] RFC for edk2-tools-library v2 previously known as RFC for Edk2-Library

One final update as requested.   Changing the repo name and pip module to edk2-pytool-library


[-- Attachment #2: Type: text/html, Size: 40152 bytes --]

Re: [edk2-devel] RFC for Edk2-Library

[kondal.r.purma]

Hi Sean ,

Its great that all python files must pass flake8 Python Style. I remember flake8 does not show errors for undefined member variables or instances . 

I feel this is one of most common use cases of code failures, due to typing errors and won't be visible unless  test that use case.

Are we planning to use any flake8 plug-ins to cover this or is it good idea to use Pylint (only to cover features not covered by falke8)on top of flake8.

Thanks,
Kondal.

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Michael D Kinney
Sent: Wednesday, May 22, 2019 7:32 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; sean.brogan@microsoft.com; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] RFC for Edk2-Library

Hi Sean,

Can you send an RFC V2 that makes the following changes:

1) Add use of python virtual environments
2) Update Repository name: edk2-tools-library
3) Provide a summary of the APIs/Services that this PIP module 
   provides and the APIs/Services from the edk2 repo that this
   PIP module depends on.
4) Contribution process.  Add recommendation that PRs be focused
   on changes that make sense to be squashed.  Submit a different
   PR for a different feature/issue.  Break up a complex PR into
   multiple PRs.
5) Remove the following bullet:

   "* Potentially move content from basetools/source/python/common/*"

   We can discuss this concept after TianoCore platforms are
   successfully using these new features and the overlap between
   the edk2 repo and edk2-tools-library repo is clearly understood.

A follow on task should evaluate GitHub PR options for submitting and preserving a patch series.

Thanks,

Mike


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, May 13, 2019 1:29 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; 
> devel@edk2.groups.io; sean.brogan@microsoft.com
> Subject: Re: [edk2-devel] RFC for Edk2-Library
> 
> On 05/13/19 20:20, Kinney, Michael D wrote:
> > Laszlo,
> >
> > On Windows build systems, we have to install OpenSSL
> command line
> > utilities.  For all host systems, the OpenSSL command
> line
> > utilities need to be in the system path.  My point is
> that this
> > is similar to other dependencies like iASL and NASM.
> 
> OK. I think I must have misunderstood you at some point.
> Sorry about that.
> 
> > For the patch discussion, I did not mean to confuse
> things.  From
> > one perspective, there are two types of patch
> submissions.  Single
> > commit (no series) and multiple commits (patch series).
> The squash
> > merge of a PR works just fine for the single commit (no
> series) type.
> > There may be feedback/comments that require code
> changes and once the
> > PR is accepted, the history in the repository shows a
> single commit.
> > As the PR evolves, commits are made on the PR to
> address each piece
> > of feedback.  Squashing all of these together at the
> time the PR is
> > accepted is the correct action and matches what we do
> the for email
> > based review process.  The final result for both PR and
> email is a
> > single commit with a cleaned up commit message.
> 
> OK.
> 
> > You may consider the single commit (no series) type
> more rare than the
> > multiple commit (patch series) type.  However, there
> may be cases where
> > a multiple commit (patch series) was used where the
> changes could have
> > been submitted as a set of single commit (no series)
> changes.
> 
> OK.
> 
> Thanks
> Laszlo
> 
> >
> > Best regards,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Monday, May 13, 2019 3:46 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; 
> >> devel@edk2.groups.io; sean.brogan@microsoft.com
> >> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>
> >> On 05/10/19 02:01, Kinney, Michael D wrote:
> >>> Laszlo,
> >>>
> >>> 1) We also use OpenSSL command line tool to locally
> >> sign
> >>>    capsules and recovery images for local testing.
> So
> >> both
> >>>    tool dependency and source dependency apply to the
> >> OpenSSL
> >>>    content.
> >>
> >> I haven't used the tools yet that you refer to, so I'm unsure how 
> >> exactly they invoke the "openssl" utility. If they
> just
> >> rely on the PATH
> >> environment variable, then what they invoke comes from the 
> >> system-wide openssl package.
> >>
> >>>
> >>> 2) If a dev submits a PR, and there are many review
> >> comments
> >>>    that require code changes, then those code changes
> >> are
> >>>    added to that PR until all feedback is addressed.
> >>
> >> I don't understand how. Let's say the pull request
> refers
> >> to a branch
> >> with three commits, and the majority of the review comments request 
> >> updates for patch #2 (i.e., in the middle). How can
> the
> >> submitter "add
> >> changes to the PR"? It is patch #2 that needs to be reworked, which 
> >> will require rebases, and so the hash of the HEAD commit of the 
> >> topic branch (patch #3) will change as well.
> >>
> >>>    At that
> >>>    point, if the change is something that should be
> in
> >> a single
> >>>    commit, then doing a squash merge with a cleaned
> up
> >> commit
> >>>    message would be appropriate.
> >>
> >> I don't understand -- we modify only patch #2, yes, to address 
> >> review comments, but why does that justify squashing #1
> through
> >> #3 into a
> >> single commit?
> >>
> >>>    And the history of all the
> >>>    review feedback preserved in the PR.
> >>
> >> That's good (but it could be better -- see the lacking email 
> >> integration. Anyway this is not strictly tied to my concern with 
> >> squash-on-merge).
> >>
> >>>
> >>>    If we create a 2nd PR with the cleaned up content,
> >> then the
> >>>    connection to the 1st PR feedback may be lost.  I
> >> agree this
> >>>    matches what we do in email reviews to create a V2
> >> patch series.
> >>>    The V1 and V2 threads are in the email archive.
> >>
> >> Indeed. And the blurb for both threads reference the bugzilla, and 
> >> the bugzilla has (if the submitter is careful) comments linking the 
> >> v1 and
> >> v2 threads from the email archive.
> >>
> >>>    I wonder if
> >>>    there is a way to link a 2nd PR to the 1st PR and
> >> guarantee
> >>>    that both PRs are preserved?  This would also
> allow
> >> the 2nd
> >>>    cleaned up PR to contain a series, and if there
> was
> >> a way to
> >>>    make the squash merge optional, then the developer
> >> can choose
> >>>    the way the patches are committed.
> >>
> >> This sounds great! I think both PRs can reference the bugzilla 
> >> ticket, and the bugzilla ticket can reference both PRs too (as 
> >> comments). The first PR can be rejected & closed when the second 
> >> one
> is
> >> submitted.
> >>
> >>>    Just a few ideas to explore...Perhaps Sean can
> >> provide some
> >>>    additional ideas to manage complex changes in PRs.
> >>
> >> Thanks!
> >> Laszlo
> >>
> >>>
> >>> Best regards,
> >>>
> >>> Mike
> >>>
> >>>> -----Original Message-----
> >>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >>>> Sent: Thursday, May 9, 2019 3:56 PM
> >>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; 
> >>>> devel@edk2.groups.io; sean.brogan@microsoft.com
> >>>> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>>>
> >>>> On 05/09/19 20:06, Kinney, Michael D wrote:
> >>>>> Hello,
> >>>>>
> >>>>> It is difficult to tell if the repo name edk2-
> library
> >>>> is for firmware
> >>>>> or tools, so I recommend we work on a name that
> >> clearly
> >>>> identifies
> >>>>> that this repo is related to tools.
> >>>>
> >>>> Good idea.
> >>>>
> >>>>> For the pip dependencies, is the concern that a
> >>>> platform that depends
> >>>>> on these tools will not be buildable without
> running
> >> a
> >>>> "pip install"
> >>>>> command that pulls content from the network?
> >>>>
> >>>> Almost. Not exactly.
> >>>>
> >>>> First, the concern is not specific to any given
> >> platform.
> >>>> My
> >>>> understanding is that the extraction would target,
> in
> >> the
> >>>> longer term,
> >>>> general BaseTools functionality. That would impact
> all
> >>>> platforms that
> >>>> consume & build against edk2.
> >>>>
> >>>> Second, it's not the network access that's
> concerning
> >> per
> >>>> se. It's the
> >>>> compatibility / interoperation with any given Linux 
> >>>> distribution's own
> >>>> (native) package management system. Such a package management 
> >>>> system is generally not used on Windows, therefore Windows
> users
> >>>> are accustomed to
> >>>> installing software packages from a boatload of
> >> vendors.
> >>>> This is not the
> >>>> case on Linux distros -- your distro vendor offers a curated set 
> >>>> of packages, such that everything interoperates with everything 
> >>>> (ideally), there are no file conflicts, version dependencies
> are
> >>>> handled
> >>>> automatically (e.g. if you install a high-level
> >> package,
> >>>> its
> >>>> dependencies are pulled in automatically), and so
> on.
> >>>>
> >>>> Clearly the distro vendor does not *author* all this software 
> >>>> (although they are strongly encouraged to contribute to the 
> >>>> upstream software projects they package and ship). The distro 
> >>>> vendor
> is
> >>>> responsible for
> >>>> the integration of all these packages into a
> >> consistent
> >>>> OS, into
> >>>> consistent feature sets, and so on.
> >>>>
> >>>> "pip" and similar tools are generally unfit for this approach, 
> >>>> because they implement a parallel package management system,
> >> to
> >>>> my
> >>>> understanding. If they are carefully used in a
> user's
> >>>> home directory,
> >>>> they might work. If packages were installed with
> "pip"
> >>>> system-wide, they
> >>>> would almost certainly break (conflict with) the
> >> distro-
> >>>> provided
> >>>> packages.
> >>>>
> >>>> Therefore, when users would like to get a new piece
> of
> >>>> software
> >>>> packaged, or an existent package refreshed (to a
> more
> >>>> recent upstream
> >>>> version), they file a feature requst with their
> distro
> >>>> vendor (unless
> >>>> the distro vendor already tracks the upstream
> project
> >>>> closely and
> >>>> performs periodic rebases anyway). Then the distro
> >> vendor
> >>>> ships a new
> >>>> (or refreshed) package, again nicely integrated with
> >> the
> >>>> rest of the
> >>>> system.
> >>>>
> >>>>> We already have to pull content to get the sources
> >> and
> >>>> potentially
> >>>>> other dependent tools (NASM, iASL, openssl).
> >>>>
> >>>> Yes, these are good examples to demonstrate the difference. 
> >>>> Consider NASM. If a Windows user would like to install NASM,
> >> they
> >>>> google "NASM
> >>>> for Windows", then go to:
> >>>>
> >>>>
> >>
> https://www.nasm.us/pub/nasm/releasebuilds/2.14.02/win64/
> >>>>
> >>>> download the ZIP file or EXE file, and install it.
> >>>>
> >>>> In comparison, a Fedora user runs
> >>>>
> >>>>   dnf install nasm
> >>>>
> >>>> and they get the package from the Fedora package repository. A 
> >>>> Debian user would run
> >>>>
> >>>>   apt-get install nasm
> >>>>
> >>>> and they would get the package (which would be
> >> entirely
> >>>> independent of
> >>>> Fedora's) from the Debian package repository.
> >>>>
> >>>> The various *BSDs would run a variant of
> >>>>
> >>>>   pkg_add nasm
> >>>>
> >>>> I believe.
> >>>>
> >>>> The same applies to iASL.
> >>>>
> >>>>
> >>>> OpenSSL is a special case. It is different from NASM
> >> and
> >>>> iASL. NASM and
> >>>> iASL are native (hosted) applications, so any Linux distro can 
> >>>> easily build them (for itself), and the binaries can be
> >> shipped
> >>>> to users, ready
> >>>> to run. But for the purposes of edk2, OpenSSL is not needed as a 
> >>>> native
> >>>> (hosted) utility, like NASM and iASL -- it is needed
> >> as a
> >>>> *cross-built*
> >>>> static library, targeting the firmware architecture
> >> (not
> >>>> the host
> >>>> architecture). Unfortunately, OpenSSL (the upstream
> >>>> project) does not
> >>>> fully support this use case yet, therefore Linux
> >> distros
> >>>> cannot just
> >>>> cross-compile OpenSSL for edk2, and offer static
> >> library
> >>>> packages. This
> >>>> is why upstream edk2 consumes OpenSSL through a git submodule, in 
> >>>> source form.
> >>>>
> >>>> *However*: conceptually, there is no difference. In Fedora and 
> >>>> RHEL, we don't build OVMF against upstream OpenSSL. We build
> it
> >>>> against the
> >>>> downstream OpenSSL source code. So ultimately there
> is
> >> no
> >>>> difference in
> >>>> the distro vendor's approach -- the sources and the binaries come 
> >>>> from the distro vendor.
> >>>>
> >>>>
> >>>> So what follows from this? It means that, for using BaseTools on 
> >>>> a Linux distro, users will have to install some extra Python 
> >>>> packages, in addition to NASM and iASL, from their vendor's 
> >>>> repository. In other words, the "edk2-library" (or "edk2-tools") 
> >>>> project should do upstream releases, so that Linux distros can 
> >>>> package them in
> >> their
> >>>> native package
> >>>> systems. And some collaboration between upstream and downstreams 
> >>>> is expected for this. (Exactly as it occurs with the
> iASL
> >>>> and NASM upstream
> >>>> projects.)
> >>>>
> >>>>> Can we limit the initial scope to tools that layer
> on
> >>>> top of edk2, and
> >>>>> a different future RFC would be required if there
> is
> >> a
> >>>> proposal for
> >>>>> the edk2 repo to depend on another repo?
> >>>>
> >>>> Yes, that would be very nice.
> >>>>
> >>>>> If we accept this limited scope, are there still
> >>>> concerns about
> >>>>> initially using squash commits for changes to these
> >>>> tools.
> >>>>
> >>>> Yes, I still have the same concern -- although it's
> a
> >>>> milder concern,
> >>>> dependent on how long squash merges are tolerated.
> >>>>
> >>>> Assume that a second, separate RFC gets posted down
> >> the
> >>>> road, let's say
> >>>> a year in, which proposes to replace the in-line component FooBar 
> >>>> of BaseTools, with the external project "edk2-library"
> >> (or
> >>>> "edk2-tools").
> >>>> At that point, the git history accrued thus far, in
> >> edk2-
> >>>> tools, would
> >>>> suddenly be inherited by all BaseTools users. And
> that
> >>>> history wouldn't
> >>>> be really helpful.
> >>>>
> >>>> For an example, we need not look past the edk2
> >> repository
> >>>> itself. Let's
> >>>> say I'd like to learn about the implementation
> history
> >> of
> >>>> the
> >>>> HandleProtocol() boot service, in edk2. So I run:
> >>>>
> >>>> $ git blame master --
> >> MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>>
> >>>> and I look up the CoreHandleProtocol() function, and
> >> find
> >>>> this:
> >>>>
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  931)
> >>>> EFI_STATUS
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  932)
> >> EFIAPI
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  933)
> >>>> CoreHandleProtocol (
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  934)
> IN
> >>>> EFI_HANDLE       UserHandle,
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  935)
> IN
> >>>> EFI_GUID         *Protocol,
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  936)
> >> OUT
> >>>> VOID            **Interface
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  937)
> )
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  938) {
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  939)
> >>>> return CoreOpenProtocol (
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  940)
> >>>> UserHandle,
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  941)
> >>>> Protocol,
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  942)
> >>>> Interface,
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  943)
> >>>> gDxeCoreImageHandle,
> >>>>> 022c6d45ef786 MdeModulePkg/Core/Dxe/Hand/Handle.c
> >>>> (qhuang8          2008-07-24 02:54:45 +0000  944)
> >>>> NULL,
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  945)
> >>>> EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  946)
> >>>> );
> >>>>> 28a00297189c3 MdeModulePkg/Core/Dxe/Hand/handle.c
> >>>> (yshang1          2007-07-04 10:51:54 +0000  947) }
> >>>>
> >>>> Interesting, it seems like the function was
> originally
> >>>> added in commit
> >>>> 28a00297189c3, and then the argument list was
> modified
> >> in
> >>>> commit
> >>>> 022c6d45ef786, a year later. So let's check out the rationale for 
> >>>> that argument list update:
> >>>>
> >>>> $ git show --color 022c6d45ef786
> >>>>
> >>>> Well... The commit message says, "Code Scrub for Dxe Core". 
> >>>> That's all.
> >>>> We also learn it comes from SVN revision 5560. And
> the
> >>>> diffstat for the
> >>>> patch is:
> >>>>
> >>>>>  38 files changed, 2499 insertions(+), 2504
> >> deletions(-
> >>>> )
> >>>>
> >>>> It is not overly helpful.
> >>>>
> >>>> Clearly, CoreHandleProtocol() is a super-robust,
> >> super-
> >>>> mature function,
> >>>> so we don't really care for its development history today.
> >>>>
> >>>> I believe the same would not apply to the "edk2-
> >> library"
> >>>> (or
> >>>> "edk2-tools") project. When it replaced the in-line component 
> >>>> FooBar of BaseTools, it's plausible it would be suddenly
> exposed
> >> to
> >>>> a larger
> >>>> userbase. New issues would be encountered, and
> people
> >>>> might want to look
> >>>> at the git history -- if for nothing else, then
> >> rationale
> >>>> (commit
> >>>> messages) on small parts of the code.
> >>>>
> >>>>
> >>>> I can also name more recent examples. I'm not a
> >> frequent
> >>>> BaseTools
> >>>> contributor, but it has happened occasionally. (I've
> >> got
> >>>> 41 patches in
> >>>> the edk2 git history that touched BaseTools/, as of commit
> >>>> 0a506fc7ab8b.)
> >>>>
> >>>> * For
> >>>>
> <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>,
> >> I
> >>>> pushed
> >>>>   5+1 patches:
> >>>>
> >>>>     67983484a443 BaseTools/footer.makefile: expand BUILD_CFLAGS 
> >>>> last for C files too
> >>>>     03252ae287c4 BaseTools/header.makefile: remove
> "-
> >> c"
> >>>> from BUILD_CFLAGS
> >>>>     b8a661702643 BaseTools/Source/C: split "-O2" to 
> >>>> BUILD_OPTFLAGS
> >>>>     b0ca5dae78ff BaseTools/Source/C: take
> >> EXTRA_OPTFLAGS
> >>>> from the caller
> >>>>     81502cee20ac BaseTools/Source/C: take
> >> EXTRA_LDFLAGS
> >>>> from the caller
> >>>>     +
> >>>>     aa4e0df1f0c7 BaseTools/VfrCompile: honor EXTRA_LDFLAGS
> >>>>
> >>>> * For
> >>>>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>,
> >> I
> >>>> pushed 26
> >>>>   patches:
> >>>>
> >>>>     8ff122119941 EmulatorPkg: require GCC48 or later
> >>>>     8d7cdfae8cb8 OvmfPkg: require GCC48 or later
> >>>>     fd158437dcce Vlv2TbltDevicePkg: assume GCC48 or
> >> later
> >>>>     7a9dbf2c94d1 BaseTools/Conf/tools_def.template:
> >> drop
> >>>> ARM/AARCH support from GCC46/GCC47
> >>>>     48e64498c961 BaseTools/tools_def.template: fix
> up
> >> LF-
> >>>> only line terminator
> >>>>     7381a6627a66 BaseTools/tools_def.template: strip trailing 
> >>>> whitespace
> >>>>     9bbf156faaad BaseTools/tools_def.template:
> remove
> >>>> GCC48_IA32_X64_DLINK_COMMON dead-end
> >>>>     3c5613c5932c BaseTools/tools_def.template:
> remove
> >>>> GCC47 leaf definitions
> >>>>     fc87b8d7f411 BaseTools/tools_def.template:
> >> propagate
> >>>> loss of GCC47 references
> >>>>     91a67e0f111e BaseTools/tools_def.template:
> remove
> >>>> GCC47 documentation
> >>>>     0f234fb8a662 BaseTools/tools_def.template:
> remove
> >>>> GCC46 leaf definitions
> >>>>     83a8f313884a BaseTools/tools_def.template:
> >> propagate
> >>>> loss of GCC46 references
> >>>>     be359fa7ceec BaseTools/tools_def.template:
> remove
> >>>> GCC46 documentation
> >>>>     1458af0cbce0 BaseTools/tools_def.template:
> remove
> >>>> GCC45 leaf definitions
> >>>>     024576896d42 BaseTools/tools_def.template:
> >> propagate
> >>>> loss of GCC45 references
> >>>>     3e77d20f5cb3 BaseTools/tools_def.template:
> remove
> >>>> GCC45 documentation
> >>>>     e046dc60fb89 BaseTools/tools_def.template:
> remove
> >>>> GCC44 leaf definitions
> >>>>     38c570efede0 BaseTools/tools_def.template:
> >> propagate
> >>>> loss of GCC44 references
> >>>>     383d29096846 BaseTools/tools_def.template:
> rename
> >>>> GCC44_ALL_CC_FLAGS to GCC48_ALL_CC_FLAGS
> >>>>     0db91daf5228 BaseTools/tools_def.template:
> >> eliminate
> >>>> GCC44_IA32_X64_DLINK_FLAGS
> >>>>     84d21abf4e36 BaseTools/tools_def.template:
> rename
> >>>> GCC44_IA32_X64_DLINK_COMMON to
> >>>> GCC48_IA32_X64_DLINK_COMMON
> >>>>     5c6ccd53244b BaseTools/tools_def.template:
> remove
> >>>> comment about GCC44 + LzmaF86Compress
> >>>>     3bc65326d6ed BaseTools/tools_def.template:
> remove
> >>>> GCC44 documentation
> >>>>     f7282023e758 ArmPkg/ArmSoftFloatLib: drop build
> >> flags
> >>>> specific to GCC46/GCC47
> >>>>     300b8c5f150c CryptoPkg/BaseCryptLib: drop build
> >> flags
> >>>> specific to GCC44
> >>>>     7423ba9d499b Revert "MdePkg: avoid
> >>>> __builtin_unreachable() on GCC v4.4"
> >>>>
> >>>>   (Note, from this list, 7a9dbf2c94d1 was authored
> by
> >>>> Ard, I just
> >>>>   included it at the right spot.)
> >>>>
> >>>> All of these patch series were carefully structured,
> >> and
> >>>> carefully
> >>>> documented (in the commit messages). I would have
> been
> >>>> devastated, had
> >>>> they been squashed.
> >>>>
> >>>> (And it would have been questionable to squash Ard's patch with 
> >>>> patches authored by me.)
> >>>>
> >>>>
> >>>>> Is there a way for GitHub to support both squash
> >>>> commits for some PRs
> >>>>> and preserve a patch series for other PRs?
> >>>>
> >>>> I don't know.
> >>>>
> >>>> But honestly, what is the purpose of squash merges?
> >> Don't
> >>>> we expect
> >>>> *all* contributions in well structured patch series?
> >>>>
> >>>> If review discovers an issue with the structure of
> the
> >>>> series (for
> >>>> example, not buildable in the middle, or the steps
> are
> >>>> not logical in
> >>>> that order, or some patches do too many things at
> once
> >>>> and should be
> >>>> split up, or a patch in the middle has a typo), the
> >> fixes
> >>>> shouldn't be
> >>>> just heaped on top. The submitter should restructure
> >>>> (rebase) the
> >>>> series, and submit the reworked series on a new
> topic
> >>>> branch / pull
> >>>> request.
> >>>>
> >>>> Isn't this the contribution pattern we'd like to see
> >> in
> >>>> all
> >>>> sub-projects, where we (the TianoCore community)
> have
> >>>> reviewer /
> >>>> maintainer responsibilities?
> >>>>
> >>>> Thanks,
> >>>> Laszlo
> >>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Mike
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: devel@edk2.groups.io
> >>>> [mailto:devel@edk2.groups.io]
> >>>>>> On Behalf Of Laszlo Ersek
> >>>>>> Sent: Wednesday, May 8, 2019 2:56 AM
> >>>>>> To: devel@edk2.groups.io;
> sean.brogan@microsoft.com
> >>>>>> Subject: Re: [edk2-devel] RFC for Edk2-Library
> >>>>>>
> >>>>>> On 05/07/19 23:35, Sean via Groups.Io wrote:
> >>>>>>> RFC  Edk2-Library creation
> >>>>>>>
> >>>>>>> Create a new tianocore owned repository to host a
> >>>>>> python library
> >>>>>>> package in support of UEFI development.  This
> >> package
> >>>>>> will allow easy
> >>>>>>> sharing of python library code to facilitate
> reuse.
> >>>>>> Inclusion of this
> >>>>>>> package and dependency management should be
> managed
> >>>>>> using Pip/Pypi. To
> >>>>>>> start this is a supplemental package and is not
> >>>>>> required to be used
> >>>>>>> for edk2 builds.
> >>>>>>
> >>>>>> [1]
> >>>>>>
> >>>>>>> Examples of content here
> >>>>>>>
> >>>>>>> * Edk2 file type parsing
> >>>>>>> * UEFI structure encode/decode in python
> >>>>>>> * Packaging tools (Capsules generation, signing,
> >> INF
> >>>>>> gen, Cat gen)
> >>>>>>> * TPM support code
> >>>>>>> * Potentially move content from
> >>>>>> basetools/source/python/common/*
> >>>>>>
> >>>>>> [2]
> >>>>>>
> >>>>>>> * No command line tools/interface
> >>>>>>>
> >>>>>>> Maintainers
> >>>>>>>
> >>>>>>> * Sean Brogan
> >>>>>>> * Bret Barkelew
> >>>>>>> * Placeholder for existing maintainer from the
> >>>>>> basetools
> >>>>>>>
> >>>>>>> License
> >>>>>>>
> >>>>>>> * BSD + Patent (edk2 aligned)
> >>>>>>>
> >>>>>>> Contribution process and issue tracking
> >>>>>>>
> >>>>>>> * Follow Github PR process for contributions and
> >>>> issue
> >>>>>> tracking
> >>>>>>> * Contributor forks repo in github
> >>>>>>> * Contributor creates branch for work
> >>>>>>> * Contributor updates release notes to indicate
> >>>> change
> >>>>>> (if necessary)
> >>>>>>> * Contributor submits PR to master branch of
> >>>>>> tianocore/Edk2-Library
> >>>>>>>   repo
> >>>>>>> * Review feedback is given in PR
> >>>>>>> * Python Tests are run on the repo (new
> >> contributions
> >>>>>> need unit tests)
> >>>>>>> * Python Style (flake8) must pass
> >>>>>>> * All review feedback must be completed,
> >> maintainers
> >>>>>> approved, and
> >>>>>>>   tests run successfully before PR is *squash
> >> merged*
> >>>>>> into master
> >>>>>>
> >>>>>> The sentences
> >>>>>>
> >>>>>> [1] "To start this is a supplemental package and
> is
> >>>> not
> >>>>>> required to be
> >>>>>>      used for edk2 builds."
> >>>>>>
> >>>>>> [2] "Potentially move content from 
> >>>>>> basetools/source/python/common/*"
> >>>>>>
> >>>>>> foreshadow that such a code movement might happen
> >> down
> >>>>>> the road, and the
> >>>>>> external package could become a requirement for
> >>>> building
> >>>>>> edk2.
> >>>>>>
> >>>>>> That step would mean the following:
> >>>>>>
> >>>>>> (a) Edk2 would not remain buildable from a single command
> >>>>>>
> >>>>>>     git clone --recurse-submodules
> >>>>>>
> >>>>>>     Building edk2 would require GNU/Linux users to
> >>>> start
> >>>>>> tracking
> >>>>>>     packages with "pip", which is independent of
> any
> >>>>>> given distro's own
> >>>>>>     package management, and may cause conflicts if
> >> not
> >>>>>> used carefully:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>
> https://developer.fedoraproject.org/tech/languages/pytho
> >>>>>> n/pypi-installation.html
> >>>>>>
> >>>>>>     This requirement on "pip" would only go away
> >> once
> >>>>>> the external
> >>>>>>     python dependencies were packaged for at least
> >> the
> >>>>>> larger GNU/Linux
> >>>>>>     distros.
> >>>>>>
> >>>>>> (b) Edk2 users running into build problems related
> >> to
> >>>>>> the external
> >>>>>>     python dependencies would have to contribute
> >>>> through
> >>>>>> a github-only
> >>>>>>     workflow. That's not a deal-breaker per se --
> if
> >>>> we
> >>>>>> want to
> >>>>>>     contribute to other edk2 dependencies, such as OpenSSL or 
> >>>>>> nasm, we
> >>>>>>     also have to conform to their specific
> >> development
> >>>>>> models, clearly.
> >>>>>>
> >>>>>>     However, "squash merge" is a catastrophically
> >> bad
> >>>>>> development model,
> >>>>>>     and I'd object to introducing a new edk2 build dependency 
> >>>>>> that
> >>>>>>     followed that model.
> >>>>>>
> >>>>>>     (There are other issues with the GitHub.com development 
> >>>>>> workflow, as
> >>>>>>     discussed elsewhere, but "squash merge" takes
> >> the
> >>>>>> cake.)
> >>>>>>
> >>>>>> Problem (a) would be solvable in the long term
> >>>> (through
> >>>>>> collaboration
> >>>>>> with distro maintainers, i.e. downstream
> packaging),
> >>>> but
> >>>>>> problem (b)
> >>>>>> would not be. Thus I'm fine with the proposal, in
> >> its
> >>>>>> current form, only
> >>>>>> if the new package is 100% an addition on top of
> >> edk2,
> >>>>>> even in the long
> >>>>>> term, and not in any part intended as a future replacement for 
> >>>>>> current
> >>>>>> edk2 functionality.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Laszlo
> >>>>>>
> >>>>>>
> >>>>>
> >>>
> >>
> >>
> >> 
> >

Re: [edk2-devel] RFC for edk2-tools-library v2 previously known as RFC for Edk2-Library

[Sean]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

Final update on the RFC.

The repo is up and populated and ready for contributions.   Please read the repository readme for details.
https://github.com/tianocore/edk2-pytool-library

An initial Pypi release has been made
https://pypi.org/project/edk2-pytool-library/

And CI & pull request builds are active on Windows and Linux
https://dev.azure.com/tianocore/edk2-pytools-library/_build?definitionId=4

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 752 bytes --]