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

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

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