[edk2-devel] RFC for Edk2-Library
Michael D Kinney
michael.d.kinney at intel.com
Thu May 23 02:32:04 UTC 2019
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 at redhat.com]
> Sent: Monday, May 13, 2019 1:29 PM
> To: Kinney, Michael D <michael.d.kinney at intel.com>;
> devel at edk2.groups.io; sean.brogan at 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 at edk2.groups.io
> [mailto:devel at edk2.groups.io]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Monday, May 13, 2019 3:46 AM
> >> To: Kinney, Michael D <michael.d.kinney at intel.com>;
> >> devel at edk2.groups.io; sean.brogan at 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 at redhat.com]
> >>>> Sent: Thursday, May 9, 2019 3:56 PM
> >>>> To: Kinney, Michael D <michael.d.kinney at intel.com>;
> >>>> devel at edk2.groups.io; sean.brogan at 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 at edk2.groups.io
> >>>> [mailto:devel at edk2.groups.io]
> >>>>>> On Behalf Of Laszlo Ersek
> >>>>>> Sent: Wednesday, May 8, 2019 2:56 AM
> >>>>>> To: devel at edk2.groups.io;
> sean.brogan at 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
> >>>>>>
> >>>>>>
> >>>>>
> >>>
> >>
> >>
> >>
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41261): https://edk2.groups.io/g/devel/message/41261
Mute This Topic: https://groups.io/mt/31536886/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list