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

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

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