Date   

Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Bret Barkelew <bret.barkelew@...>
 

Rebecca,

I was able to confirm that it was an issue with Git for Windows. Looks like it’s fixed in current snapshots and will be in the next release:
https://github.com/git-for-windows/git/issues/2598

Also, ATTN: @Michael Kubacki<mailto:Michael.Kubacki@microsoft.com>

- Bret

From: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Sent: Wednesday, May 27, 2020 10:45 AM
To: Rebecca Cran<mailto:rebecca@bsdio.com>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Andrew Fish<mailto:afish@apple.com>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; spbrogan@outlook.com<mailto:spbrogan@outlook.com>; Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)<mailto:leif@nuviainc.com>
Subject: RE: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

That’s not a bad idea: I should try with my WSL install.

I’m on the same version of Git for Windows, and think I’ll open it as an issue to the maintainer.

For now, going though the paces is just as useful to me as getting a viable environment (after all, PRs soon!), so I don’t mind trying another OS or install if that’s what it takes.

- Bret

From: Rebecca Cran<mailto:rebecca@bsdio.com>
Sent: Wednesday, May 27, 2020 9:07 AM
To: rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Andrew Fish<mailto:afish@apple.com>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; spbrogan@outlook.com<mailto:spbrogan@outlook.com>; Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)<mailto:leif@nuviainc.com>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

On 5/27/2020 6:12 AM, Laszlo Ersek wrote:

So, it could be a MINGW64 packaging bug, perhaps.
I'm getting the same error, but with a different packaging of Git:
mine's in C:\Program Files\Git\cmd\git.exe .

It's version "git version 2.26.2.windows.1".

Of course it's possible it's just the same MINGW version that's been put
into its own installer.


I also tried using my openSUSE WSL installation, but it failed with:

STARTTLS failed! SSL connect attempt failed error:1416F086:SSL
routines:tls_process_server_certificate:certificate verify failed at
/usr/lib/git/git-send-email line 1548.


So I ended up copying it to one of my FreeBSD systems and sent it from
there.


--
Rebecca Cran


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Laszlo Ersek
 

On 05/28/20 00:07, Rebecca Cran wrote:

I also tried using my openSUSE WSL installation, but it failed with:

STARTTLS failed! SSL connect attempt failed error:1416F086:SSL
routines:tls_process_server_certificate:certificate verify failed at
/usr/lib/git/git-send-email line 1548.
That's different -- in this case, peer certificate verification was
attempted, but it failed, because the root certificate in the peer's
cert chain is not trusted by your system (your openSUSE WSL environment).

The fix for that should be identical to what you'd do on a standalone
openSUSE installation -- (1) figure out what CA cert is the root of the
peer's cert chain, and (2) decide consciously whether you trust that CA
cert to sign other certificates, (3) import said CA cert persistently
into your "store of trusted CA certs".

Examples:

(1) I think one command that works is:

$ openssl s_client -showcerts -connect HOST:PORT </dev/null

(2) up to you :)

(3a) On RHEL, this would mean copying the CA certificate under
"/etc/pki/ca-trust/source/anchors/", in PEM format, and then running the
"update-ca-trust extract" command. (Both actions need root (uid=0)
access, of course.)

(3b) For a user session (i.e., not system-wide), git-send-email also
takes "--smtp-ssl-cert-path":

--smtp-ssl-cert-path
Path to a store of trusted CA certificates for SMTP SSL/TLS
certificate validation (either a directory that has been
processed by c_rehash, or a single file containing one or
more PEM format certificates concatenated together: see
verify(1) -CAfile and -CApath for more information on
these). Set it to an empty string to disable certificate
verification. Defaults to the value of the
sendemail.smtpsslcertpath configuration variable, if set,
or the backing SSL library's compiled-in default otherwise
(which should be the best choice on most platforms).

Thanks
Laszlo


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Bret Barkelew <bret.barkelew@...>
 

That’s not a bad idea: I should try with my WSL install.

I’m on the same version of Git for Windows, and think I’ll open it as an issue to the maintainer.

For now, going though the paces is just as useful to me as getting a viable environment (after all, PRs soon!), so I don’t mind trying another OS or install if that’s what it takes.

- Bret

From: Rebecca Cran<mailto:rebecca@bsdio.com>
Sent: Wednesday, May 27, 2020 9:07 AM
To: rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Andrew Fish<mailto:afish@apple.com>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; spbrogan@outlook.com<mailto:spbrogan@outlook.com>; Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)<mailto:leif@nuviainc.com>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

On 5/27/2020 6:12 AM, Laszlo Ersek wrote:

So, it could be a MINGW64 packaging bug, perhaps.
I'm getting the same error, but with a different packaging of Git:
mine's in C:\Program Files\Git\cmd\git.exe .

It's version "git version 2.26.2.windows.1".

Of course it's possible it's just the same MINGW version that's been put
into its own installer.


I also tried using my openSUSE WSL installation, but it failed with:

STARTTLS failed! SSL connect attempt failed error:1416F086:SSL
routines:tls_process_server_certificate:certificate verify failed at
/usr/lib/git/git-send-email line 1548.


So I ended up copying it to one of my FreeBSD systems and sent it from
there.


--
Rebecca Cran


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Andrew Fish <afish@...>
 

Rebecca,

I cheated and used smtpServer = relay.apple.com and smtpEncryption = tls. Seems relay.apple.com does not require authentication and it just worked.

I used an internal git mailing list to figure all this out, but seems the easy button was an smtpServer setup to be easy to use with git sendmail. It might be worth while to have folks reach out inside their companies to see if there are existing known good recipes?

Thanks,

Andrew Fish

On May 27, 2020, at 3:07 PM, Rebecca Cran <rebecca@bsdio.com> wrote:

On 5/27/2020 6:12 AM, Laszlo Ersek wrote:

So, it could be a MINGW64 packaging bug, perhaps.
I'm getting the same error, but with a different packaging of Git: mine's in C:\Program Files\Git\cmd\git.exe .

It's version "git version 2.26.2.windows.1".

Of course it's possible it's just the same MINGW version that's been put into its own installer.


I also tried using my openSUSE WSL installation, but it failed with:

STARTTLS failed! SSL connect attempt failed error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed at /usr/lib/git/git-send-email line 1548.


So I ended up copying it to one of my FreeBSD systems and sent it from there.


--
Rebecca Cran





Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Rebecca Cran
 

On 5/27/2020 6:12 AM, Laszlo Ersek wrote:

So, it could be a MINGW64 packaging bug, perhaps.
I'm getting the same error, but with a different packaging of Git: mine's in C:\Program Files\Git\cmd\git.exe .

It's version "git version 2.26.2.windows.1".

Of course it's possible it's just the same MINGW version that's been put into its own installer.


I also tried using my openSUSE WSL installation, but it failed with:

STARTTLS failed! SSL connect attempt failed error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed at /usr/lib/git/git-send-email line 1548.


So I ended up copying it to one of my FreeBSD systems and sent it from there.


--
Rebecca Cran


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Laszlo Ersek
 

On 05/27/20 03:52, Bret Barkelew wrote:
So, today I followed the Wiki (that I had never seen) and now I’m staring down the barrel of this fellow…
[cid:image002.png@01D6338E.D6C64920]

[Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.
To use SSL please install IO::Socket::SSL with version>=2.007 at /usr/share/perl5/core_perl/Net/SMTP.pm line 270.]

Anyone have thoughts? I’mma go get a scotch.
I think your perl installation and your git installation may come from
different sources, and the perl install may not satisfy the git
install's dependencies.

In GNU/Linux distribution lingo, I'd call this either a distribution
error, or (maybe more precisely) a git package error. Normally the git
package should spell out the pre-requisite package names, along with the
minimum required package version(s). And the package manager should
enforce that, when installing git.

Other people appear to have encountered a similiar issue before:

https://github.com/Homebrew/homebrew-core/issues/24210#issuecomment-366831944

https://github.com/msys2/MSYS2-packages/issues/1152

https://bugs.archlinux.org/task/54326

https://bugs.archlinux.org/task/62948


When I check on my laptop now, I see:

$ rpm --query --requires git-email
[...]
perl(Net::SMTP::SSL)
[...]

and recursively,

$ rpm --query --requires perl-Net-SMTP-SSL
[...]
perl(IO::Socket::SSL)
[...]

In other words, whenever it was that I ran "yum install git-email" (for
gaining access to the "git-send-email" command), yum made sure that
"perl-Net-SMTP-SSL" and "perl-IO-Socket-SSL" would both be pulled in.

(Assuming the RPM spec files spelled out minimum versions on the
dependencies, yum would enforce those particular versions too.)

So, it could be a MINGW64 packaging bug, perhaps.

Thanks,
Laszlo


Re: PKCS7 Authenticated Variable Enrollment

Guomin Jiang
 

I am sorry that I have not the use case, and I plan to investigating it after August.

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of
divneil.r.wadhawan@intel.com
Sent: Wednesday, May 27, 2020 6:54 PM
To: Jiang, Guomin <guomin.jiang@intel.com>; rfc@edk2.groups.io
Subject: Re: [edk2-rfc] PKCS7 Authenticated Variable Enrollment

Hi Goumin,

I had discussion internally, and I got hold off tools from:
https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git.
It is generating the correct format as per
EFI_VARIABLE_AUTHENTICATION_2.

So, I thought of first validating RSA2048 Sign verification and it is failing.
I still have to figure out that. Do you have a working use case which uses
PKCS7 format and PKCS7_verify works?

Regards,
Divneil


Re: PKCS7 Authenticated Variable Enrollment

Wadhawan, Divneil R
 

Hi Goumin,

I had discussion internally, and I got hold off tools from: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git.
It is generating the correct format as per EFI_VARIABLE_AUTHENTICATION_2.

So, I thought of first validating RSA2048 Sign verification and it is failing.
I still have to figure out that. Do you have a working use case which uses PKCS7 format and PKCS7_verify works?

Regards,
Divneil


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Tomas Pilar (tpilar)
 

This will probably be down to the [send-email] section of git config, do you have smtpEncryption enabled by any chance?

You could also try updating the required package:
perl -MCPAN -e 'install "IO::Socket::SSL"'

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via groups.io
Sent: 27 May 2020 02:53
To: Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>
Cc: devel@edk2.groups.io; spbrogan@outlook.com; rfc@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) <leif@nuviainc.com>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

So, today I followed the Wiki (that I had never seen) and now I'm staring down the barrel of this fellow...
[cid:image001.png@01D63411.7C958370]

[Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.
To use SSL please install IO::Socket::SSL with version>=2.007 at /usr/share/perl5/core_perl/Net/SMTP.pm line 270.]

Anyone have thoughts? I'mma go get a scotch.

- Bret

From: Andrew Fish<mailto:afish@apple.com>
Sent: Monday, May 25, 2020 11:28 AM
To: Laszlo Ersek<mailto:lersek@redhat.com>
Cc: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; spbrogan@outlook.com<mailto:spbrogan@outlook.com>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)<mailto:leif@nuviainc.com>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process



On May 25, 2020, at 11:10 AM, Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions I
needed as a developer, as the Wiki seems to assume you just know the
Linux kernel patch process. That feels like an area we can improve.
(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FLaszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers&;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C2e084613c24f433ca0a508d800d978de%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637260281325061578&amp;sdata=nIMHQLnu8F%2F%2BTMMsLKVXbWnO6AWE9WuUu5k1TK4HgTQ%3D&amp;reserved=0

I wrote it specifically for developers & maintainers with no (or almost
no) prior git / mailing list experience. Multiple developers confirmed
later that the article had helped them.
Laszlo,

Your wiki article was very very helpful. I just could not find it from the Tianocre wiki. It would be good if we could link to it from here [1], maybe as add to this: "Are you new to using git? If so, then the New to git page may be helpful."?

There are a lot folks who use git but don't use the email based review so they have never setup git with emali before. Your wiki, plus me figuring out the magic internal SMTP reflector (I reached out on an internal git malling list) is what got me unblocked.

[1] https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Development-Process&;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C2e084613c24f433ca0a508d800d978de%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637260281325061578&amp;sdata=XPx6jrloPC9LW0iCecZuFmaz3JgjCSQYeF0PEyGW4I0%3D&amp;reserved=0

Thanks,

Andrew Fish

Thanks
Laszlo

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Bret Barkelew <bret.barkelew@...>
 

So, today I followed the Wiki (that I had never seen) and now I’m staring down the barrel of this fellow…
[cid:image002.png@01D6338E.D6C64920]

[Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.
To use SSL please install IO::Socket::SSL with version>=2.007 at /usr/share/perl5/core_perl/Net/SMTP.pm line 270.]

Anyone have thoughts? I’mma go get a scotch.

- Bret

From: Andrew Fish<mailto:afish@apple.com>
Sent: Monday, May 25, 2020 11:28 AM
To: Laszlo Ersek<mailto:lersek@redhat.com>
Cc: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; spbrogan@outlook.com<mailto:spbrogan@outlook.com>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)<mailto:leif@nuviainc.com>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process



On May 25, 2020, at 11:10 AM, Laszlo Ersek <lersek@redhat.com> wrote:

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions I
needed as a developer, as the Wiki seems to assume you just know the
Linux kernel patch process. That feels like an area we can improve.
(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FLaszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers&;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C2e084613c24f433ca0a508d800d978de%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637260281325061578&amp;sdata=nIMHQLnu8F%2F%2BTMMsLKVXbWnO6AWE9WuUu5k1TK4HgTQ%3D&amp;reserved=0

I wrote it specifically for developers & maintainers with no (or almost
no) prior git / mailing list experience. Multiple developers confirmed
later that the article had helped them.
Laszlo,

Your wiki article was very very helpful. I just could not find it from the Tianocre wiki. It would be good if we could link to it from here [1], maybe as add to this: "Are you new to using git? If so, then the New to git page may be helpful."?

There are a lot folks who use git but don't use the email based review so they have never setup git with emali before. Your wiki, plus me figuring out the magic internal SMTP reflector (I reached out on an internal git malling list) is what got me unblocked.

[1] https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Development-Process&;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C2e084613c24f433ca0a508d800d978de%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637260281325061578&amp;sdata=XPx6jrloPC9LW0iCecZuFmaz3JgjCSQYeF0PEyGW4I0%3D&amp;reserved=0

Thanks,

Andrew Fish

Thanks
Laszlo


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Bret Barkelew <bret.barkelew@...>
 

Samer,

Have you had a chance to review Mike’s PR process? Any thoughts as comparison?

- Bret
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Samer El-Haj-Mahmoud via groups.io <samer.el-haj-mahmoud=arm.com@groups.io>
Sent: Tuesday, May 26, 2020 7:39:55 AM
To: rfc@edk2.groups.io <rfc@edk2.groups.io>; lersek@redhat.com <lersek@redhat.com>; Andrew Fish <afish@apple.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; devel@edk2.groups.io <devel@edk2.groups.io>; spbrogan@outlook.com <spbrogan@outlook.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) <leif@nuviainc.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

I agree with Andrew. I also found Laszlo's "unkempt guide" very useful. In addition, there is a short page by Peter Batard that adds more details on the commits validation, patchset generation, and e-mail submission: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgist.github.com%2Fpbatard%2Fec1c9d1dd6e7144b07a09b057b1735a8&;data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdca587d1198049354a6f08d80182b15a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637261008123224059&amp;sdata=e%2Bk1gQubOWY8gWlQAUAmjIIaqQMv6p%2FMqjUHcntVm1g%3D&amp;reserved=0


-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Laszlo Ersek
via groups.io
Sent: Tuesday, May 26, 2020 7:18 AM
To: Andrew Fish <afish@apple.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; devel@edk2.groups.io;
spbrogan@outlook.com; rfc@edk2.groups.io; Desimone, Nathaniel L
<nathaniel.l.desimone@intel.com>; Mike Kinney
<michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)
<leif@nuviainc.com>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based
Code Review Process

On 05/25/20 20:28, Andrew Fish wrote:


On May 25, 2020, at 11:10 AM, Laszlo Ersek <lersek@redhat.com> wrote:

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions
I needed as a developer, as the Wiki seems to assume you just know
the Linux kernel patch process. That feels like an area we can improve.
(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FLaszlo%27s-unkemp&;data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdca587d1198049354a6f08d80182b15a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637261008123224059&amp;sdata=hwAXd7kabi4mQyTEr7AWlEyA4yHDkdwG8zr1lirgmA4%3D&amp;reserved=0
t-git-guide-for-edk2-contributors-and-maintainers

I wrote it specifically for developers & maintainers with no (or
almost
no) prior git / mailing list experience. Multiple developers
confirmed later that the article had helped them.
Laszlo,

Your wiki article was very very helpful. I just could not find it from the
Tianocre wiki. It would be good if we could link to it from here [1], maybe as
add to this: "Are you new to using git? If so, then the New to git page may be
helpful."?

The article at [1] is an official document, while the "unkempt guide" is not
official. The unkempt guide starts by deferring to [1]. I didn't think the official
document should point to my unofficial one, and/or we should create a loop
of links.

That said, if someone else updates [1] with a pointer, I won't protest.
That's just something that I (having authored the unkempt guide) would not
propose myself.

I do agree that the wiki search facilities on github are basic. What has mostly
worked for me is clicking the Pages arrow, and then entering a *very simple*
search term in the drop-down search box. For example, if I do that now, and
only enter "git", then the "unkempt guide" is listed (with other hits of
course). I think this search box is basically for searching article titles.


There are a lot folks who use git but don't use the email based review so
they have never setup git with emali before. Your wiki, plus me figuring out
the magic internal SMTP reflector (I reached out on an internal git malling list)
is what got me unblocked.

It's great that you have access to such infrastructure at Apple!

Thanks!
Laszlo



[1]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Developme&;data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdca587d1198049354a6f08d80182b15a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637261008123234063&amp;sdata=UqY5uoxqMamf5PkFLOJ20YKE1aWTZRqGnEYuK93AxiA%3D&amp;reserved=0
nt-Process

Thanks,

Andrew Fish

Thanks
Laszlo

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Tomas Pilar (tpilar)
 

I actually agree with you, when we migrated from reviewboard to github pull requests, I was sorely disappointed with the PR functionality and ergonomics.

Tomas Pilar

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Rebecca Cran via groups.io
Sent: 14 May 2020 22:47
To: rfc@edk2.groups.io; bret.barkelew@microsoft.com; Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; lersek@redhat.com
Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

On 5/14/20 3:26 PM, Bret Barkelew via groups.io wrote:

I feel like this process is a good compromise. It�s not perfect (frankly, I�m a fan of enforced squash merges, which can maintain bisectability if managed well), but it allows for rapid iteration, ease of contribution, and approaches the workflow that many who have never used email to maintain a project would be familiar with.

It�s code management for the Instagram generation, and I for one welcome our new insect overlords.
Or at least, that's what Microsoft is betting on! :D

Personally, I remain unconvinced about the usability of Github Pull Requests for a project the size of EDK2, but I hope to be proven wrong.


--
Rebecca Cran





IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Samer El-Haj-Mahmoud
 

I agree with Andrew. I also found Laszlo's "unkempt guide" very useful. In addition, there is a short page by Peter Batard that adds more details on the commits validation, patchset generation, and e-mail submission: https://gist.github.com/pbatard/ec1c9d1dd6e7144b07a09b057b1735a8

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Laszlo Ersek
via groups.io
Sent: Tuesday, May 26, 2020 7:18 AM
To: Andrew Fish <afish@apple.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; devel@edk2.groups.io;
spbrogan@outlook.com; rfc@edk2.groups.io; Desimone, Nathaniel L
<nathaniel.l.desimone@intel.com>; Mike Kinney
<michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)
<leif@nuviainc.com>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based
Code Review Process

On 05/25/20 20:28, Andrew Fish wrote:


On May 25, 2020, at 11:10 AM, Laszlo Ersek <lersek@redhat.com> wrote:

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions
I needed as a developer, as the Wiki seems to assume you just know
the Linux kernel patch process. That feels like an area we can improve.
(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkemp
t-git-guide-for-edk2-contributors-and-maintainers

I wrote it specifically for developers & maintainers with no (or
almost
no) prior git / mailing list experience. Multiple developers
confirmed later that the article had helped them.
Laszlo,

Your wiki article was very very helpful. I just could not find it from the
Tianocre wiki. It would be good if we could link to it from here [1], maybe as
add to this: "Are you new to using git? If so, then the New to git page may be
helpful."?

The article at [1] is an official document, while the "unkempt guide" is not
official. The unkempt guide starts by deferring to [1]. I didn't think the official
document should point to my unofficial one, and/or we should create a loop
of links.

That said, if someone else updates [1] with a pointer, I won't protest.
That's just something that I (having authored the unkempt guide) would not
propose myself.

I do agree that the wiki search facilities on github are basic. What has mostly
worked for me is clicking the Pages arrow, and then entering a *very simple*
search term in the drop-down search box. For example, if I do that now, and
only enter "git", then the "unkempt guide" is listed (with other hits of
course). I think this search box is basically for searching article titles.


There are a lot folks who use git but don't use the email based review so
they have never setup git with emali before. Your wiki, plus me figuring out
the magic internal SMTP reflector (I reached out on an internal git malling list)
is what got me unblocked.

It's great that you have access to such infrastructure at Apple!

Thanks!
Laszlo



[1]
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Developme
nt-Process

Thanks,

Andrew Fish

Thanks
Laszlo

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Laszlo Ersek
 

On 05/25/20 20:28, Andrew Fish wrote:


On May 25, 2020, at 11:10 AM, Laszlo Ersek <lersek@redhat.com> wrote:

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions I
needed as a developer, as the Wiki seems to assume you just know the
Linux kernel patch process. That feels like an area we can improve.
(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

I wrote it specifically for developers & maintainers with no (or almost
no) prior git / mailing list experience. Multiple developers confirmed
later that the article had helped them.
Laszlo,

Your wiki article was very very helpful. I just could not find it from the Tianocre wiki. It would be good if we could link to it from here [1], maybe as add to this: "Are you new to using git? If so, then the New to git page may be helpful."?
The article at [1] is an official document, while the "unkempt guide" is
not official. The unkempt guide starts by deferring to [1]. I didn't
think the official document should point to my unofficial one, and/or we
should create a loop of links.

That said, if someone else updates [1] with a pointer, I won't protest.
That's just something that I (having authored the unkempt guide) would
not propose myself.

I do agree that the wiki search facilities on github are basic. What has
mostly worked for me is clicking the Pages arrow, and then entering a
*very simple* search term in the drop-down search box. For example, if I
do that now, and only enter "git", then the "unkempt guide" is listed
(with other hits of course). I think this search box is basically for
searching article titles.


There are a lot folks who use git but don't use the email based review so they have never setup git with emali before. Your wiki, plus me figuring out the magic internal SMTP reflector (I reached out on an internal git malling list) is what got me unblocked.
It's great that you have access to such infrastructure at Apple!

Thanks!
Laszlo



[1] https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Thanks,

Andrew Fish

Thanks
Laszlo


[RFC]: StandAloneMM in OP-TEE

Sahil Malhotra <sahil.malhotra@...>
 

Hi All,


We are working on enabling the Secure Storage of Variables for Secure UEFI.

Let me give you a brief idea of what we are doing.

 

We need StandAlone Management Mode to run in the Secure Environment.

For that in EDK2 we already have the MmCommunicationDxe Runtime Driver which communicates with StMM running in Secure Partition in Secure World.

But this driver is based on SPM (Secure Partition Manager) running in the ATF.

 

As we are aware that ATF can run either in SPM mode or SPD mode, Both are mutually exclusive.

So we cannot have both the StMM running in Secure Partition as well as OP-TEE or any other Secure OS running in Secure World.

 

On many systems, there are many other secure operations needed to be done that can be done by Secure OS only.

So in these systems we need to make the StMM and Secure OS work together.

 

As part of doing this only, we have created a kind of Secure Partition within OP-TEE in which StMM can work as a kind of TA, and other TAs cannot interfere with the working of this Secure Partition.

Other TAs can run in parallel to StMM on top of OP-TEE, We can get the work done by StMM by OP-TEE SMC calls only.

It will be like this as shown in the below image.

 

                                                     Secure World

+-----------------------------------------------------------+

|   +-----------------------+    +------------------------+ |

|   |                       |    | +--------------------+ | |

|   |                       |    | |                    | | |

|   |   Trusted Application |    | |                    | | |

|   |                       |    | |                    | | |

|   |                       |    | |      StMM          | | |

|   +-----------------------+    | |                    | | |

|                                | |                    | | |

|                                | |                    | | |

|                                | +--------------------+ | |

|                                |                        | |

|                  OP-TEE        |                        | |

|                                |                        | |

|                                |     Secure Partition   | |

|                                |                        | |

|                                |                        | |

|                                |                        | |

|                                +------------------------+ |

+-----------------------------------------------------------+

 

So with this approach of running StMM in an exclusive secure partition with OP-TEE, StMM and TAs can work together.

In this way StMM binary which is compiled is also environment-agnostic, Same StMM binary can work whether it is running as part of OP-TEE or Standalone in Secure Word.

If OP-TEE is responsible for running StMM, firmware implementations, like U-Boot, can use it to store UEFI variables.


Now for implementing the whole system of Secure Variable Storage for Secure UEFI.

 

We need to make changes in MmCommunicationDxe Runtime Driver and OpteeLib

Let's discuss one by one the changes:

  • MmCommunicationDxe – Currently it is based on SPM SMC calls that get landed into ATF and do the required work.
    • Now since StMM is running as part of OP-TEE, we need to change these into OP-TEE SMC calls.
  • OpteeLib – Currently OpteeLib cannot be used with the Runtime Drivers.
    • We need to change its configuration so that it can be used with a Runtime Driver.

 

So for making these changes we have following approaches:

  • MmCommunicationDxe
    • We can have the code for SPM SMC calling and OPTEE SMC calling in the same driver under some compile time flags, but it will make the code nasty.
    • Another approach can be writing a new driver under the name of MmCommunicationOpteeDxe in ArmPkg/Drivers/.
  • OpteeLib
    • We can make necessary changes to make it work with Runtime Driver.
    • Other approach we can make Runtime Driver of Optee also in ArmPkg/Drivers/

 

So please review the mail and approaches for making the changes and let us know your views.


Regards,

Sahil Malhotra


Re: PKCS7 Authenticated Variable Enrollment

Guomin Jiang
 

Hi Wadhawan,

I will check it after August because I am busy recently.

Best Regards
Guomin

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Wadhawan,
Divneil R
Sent: Friday, May 1, 2020 2:19 AM
To: rfc@edk2.groups.io
Cc: Wadhawan, Divneil R <divneil.r.wadhawan@intel.com>
Subject: [edk2-rfc] PKCS7 Authenticated Variable Enrollment

Hi,

I am trying to test if the enrollment of Authenticated Variables is possible
with verification of signature with different key sizes.
File: AuthService.c :: Function: VerifyTimeBasedPayload() -> Pkcs7Verify() So,
to achieve that, I did the following:

a. Generate the KEK.auth
uuidgen --random > GUID.txt

openssl req -newkey rsa:3072 -nodes -keyout PK.key -new -x509 -sha384 -
days 3650 -subj "/CN=my Platform Key/" -out PK.crt
openssl x509 -outform DER -in PK.crt -out PK.cer

openssl req -newkey rsa:3072 -nodes -keyout KEK.key -new -x509 -sha384
-days 3650 -subj "/CN=my Key Exchange Key/" -out KEK.crt
openssl x509 -outform DER -in KEK.crt -out KEK.cer
cert-to-efi-sig-list -g "$(< GUID.txt)" KEK.crt KEK.esl
sign-efi-sig-list -g "$(< GUID.txt)" -k PK.key -c PK.crt KEK KEK.esl KEK.auth

b. Enroll PK.cer from BIOS menu in custom mode

c. Hack the KEK enrollment connected to BIOS Menu (HII interface) and
disable custom mode and then gRT->SetVariable() on input data
File: SecureBootConfigImpl.c :: Function: EnrollKeyExchangeKey()

I found that the KEK.auth format is rejected by VerifyTimeBasedPayload() in
(CompareMem (SigData + 13, &mSha256OidValue, sizeof
(mSha256OidValue)) != 0)) {

So, I did a hexdump of KEK.auth and could see that data is not as expected.
Here's the dump I have of KEK.auth

//EFI_Time (16 bytes)
0000000 e5 07 03 10 0e 39 00 00 00 00 00 00 00 00 00 00

// WIN_CERTIFICATE_UEFI_GUID (16bytes + 8bytes) // SigData starts at value
0x30 post WIN_CERTIFICATE_UEFI_GUID. At offset of 13 from 0x30 the code
expects sha256 OID
0000020 0e 07 00 00 00 02 f1 0e 9d d2 af 4a df 68 ee 49
0000040 8a a9 34 7d 37 56 65 a7 30 82 06 f2 06 09 2a 86

0000060 48 86 f7 0d 01 07 02 a0 82 06 e3 30 82 06 df 02

// sha256 oid starts from here: 60 86 48 01 65 03 04 02 (last 8 bytes + 1byte in
next line)
0000100 01 01 31 0f 30 0d 06 09 60 86 48 01 65 03 04 02
0000120 01 05 00 30 0b 06 09 2a 86 48 86 f7 0d 01 07 01

So, the sha256 oid is present but is at a far away offset. Can you please help
in identifying the problem component.
I also wanted to talk about the oid value of sha256 present, where as sha384
is used.
The above data dump is same in terms of offset for RSA2048/SHA256
KEK.auth I have based this method on the understanding that, if we have
some OS interface to set the variables, it will pass the KEK.auth directly into
gRT->SetVariable().

Regards,
Divneil


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Andrew Fish <afish@...>
 

On May 25, 2020, at 11:10 AM, Laszlo Ersek <lersek@redhat.com> wrote:

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions I
needed as a developer, as the Wiki seems to assume you just know the
Linux kernel patch process. That feels like an area we can improve.
(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

I wrote it specifically for developers & maintainers with no (or almost
no) prior git / mailing list experience. Multiple developers confirmed
later that the article had helped them.
Laszlo,

Your wiki article was very very helpful. I just could not find it from the Tianocre wiki. It would be good if we could link to it from here [1], maybe as add to this: "Are you new to using git? If so, then the New to git page may be helpful."?

There are a lot folks who use git but don't use the email based review so they have never setup git with emali before. Your wiki, plus me figuring out the magic internal SMTP reflector (I reached out on an internal git malling list) is what got me unblocked.

[1] https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Thanks,

Andrew Fish

Thanks
Laszlo


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Laszlo Ersek
 

Hi Andrew,

On 05/25/20 06:09, Andrew Fish wrote:

I also found I had to Bing/Google to find the detailed instructions I
needed as a developer, as the Wiki seems to assume you just know the
Linux kernel patch process. That feels like an area we can improve.
(apologies if I've lost context; please disregard my message below in
that case).

I wrote the following wiki article originally in 2016:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

I wrote it specifically for developers & maintainers with no (or almost
no) prior git / mailing list experience. Multiple developers confirmed
later that the article had helped them.

Thanks
Laszlo


Re: [EXTERNAL] [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Andrew Fish <afish@...>
 

On May 21, 2020, at 10:48 PM, Bret Barkelew <Bret.Barkelew@microsoft.com> wrote:

“But we do have tools that insert the Bugzilla number in all the commits of the series, assist with the squash, etc.”

Are these internal-only, or are they something we could evaluate when we move to the PR process? If not, are they based on anything we could leverage?

I believe that the plan is to stick with Bugzilla for the immediate future rather than use GitHub Issues (which is probably for the best for now, given the span across repos and access levels), so any tooling to tie that together would be interesting to evaluate.
Bret,

Sorry for being confusing, and lazy.....

The lazy part is in house we have a bug tracking tool called radar, so I just replaced radar with BZ to make a general point.

So the scripts I mentioned are useless for us as they 100% rely on massive amounts of internal framework, and are hard coded to our current process.

Probably better to tell the story... So we revamped our internal process and that was lead by the folks who make the OS (so kind of like our edk2 process derived from the Linux kernel). So building the OS made sense, but developers got stuck doing a bunch of manual work. The response was to get a group of smart folks together and write good documentation, and build tools to automate common task. That seems like a good plane for TianoCore too?

So I finally tracked down on our internal git mailing and figured out the mail reflector I needed to follow the basic edk2 rules for patches. To me it seems like we could try and automate thing more. I also found I had to Bing/Google to find the detailed instructions I needed as a developer, as the Wiki seems to assume you just know the Linux kernel patch process. That feels like an area we can improve.

So this makes a couple of ideas pop into my head:
1) It would be good for folks that are not conversant in the Linux mailing list process to give feedback on all the Wikis.
2) Can we make a mail reflector that makes it easier to contribute? The hardest thing for me was tracking down my internal git work group that had setup for how to configure a mail server. Is there a way to help others with that?
3) We want to make sure any new process makes it as easy as possible to contribute.

I'm reminded of the epiphany I got reading Code Complete the 1st time. The data shows the most important thing is consistency. So I'd say our reluctance to change in rooted in the science of computer science but progresses is always our goal.

Thanks,

Andrew Fish

<Tangent>
In Mu we have a similar problem of keeping track of what features/bugs have already been upstreamed and when can they be dropped during an upstream integration, so that’s the more personal interest I have in such automation.

Thanks!

- Bret

From: Andrew Fish <mailto:afish@apple.com>
Sent: Thursday, May 21, 2020 8:00 PM
To: Laszlo Ersek <mailto:lersek@redhat.com>
Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; spbrogan@outlook.com <mailto:spbrogan@outlook.com>; rfc@edk2.groups.io <mailto:rfc@edk2.groups.io>; Desimone, Nathaniel L <mailto:nathaniel.l.desimone@intel.com>; Bret Barkelew <mailto:Bret.Barkelew@microsoft.com>; Kinney, Michael D <mailto:michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) <mailto:leif@nuviainc.com>
Subject: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process



On May 21, 2020, at 6:30 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On 05/20/20 00:25, Sean wrote:
On 5/19/2020 1:41 PM, Laszlo Ersek wrote:
Your proposal to "don't exclude squash merge workflows" is a trap. If
we tolerate that option -- which is obviously the sloppy, and hence
more convenient, option for some maintainers and some contributors,
to the detriment of the git history --, then almost every core
maintainer will use it as frequently as they can. In the long term,
that will hurt many consumers of the core code. It will limit the
ability of people not regularly dealing with a particular core module
to file a fine-grained bug report for that module, maybe even propose
a fix. From the regression analyst's side, if the bug report starts
with "I have a bisection log", that's already a good day. And your
proposal would destroy that option, because maintainers and people in
general are irrepairably lazy and undisciplined. We cannot post a
community member shoulder-by-shoulder with every core package
reviewer/maintainer to prevent the latter from approving a
squash-on-merge, out of pure laziness. I'm 100% sure the "option" to
squash-on-merge would *immediately* be abused for a lot more than
just "typo fixes". There isn't enough manpower to watch the watchers,
so "no squash-on-merge" needs to be a general rule.

I have trouble with this line of thinking. The maintainers are and
should be considered the representatives of this code base. They
have a vested interest to enable this repository to work for them. If
they really are viewed as "sloppy" or "lazy" then we are destined to
fail anyway.
You put it very well. "They have a vested interest to enable this
repository to work for them." Key part being "*for them*".

Core maintainers are responsible for making this repository work for a
lot larger camp than just themselves. Even if squash-on-merge satisfied
the requirements that core maintainers presented, squash-on-merge would
still hurt the larger community that depends on those packages.

The core-consumer community may not necessarily participate in the
day-to-day maintenance of the core packages, but they do report bugs and
even contributes bugfixes / occasional features, when their particular
use cases require those actions.

And squash-on-merge hurts those activities, down the road, because the
git history is instrumental to analyzing and learning the code base.

For example, the question "why do we call this function here?"
immediately leads to running "git blame" (possibly a series of git-blame
commands, to navigate past code movements and such). In the end
git-blame leads to a particular commit, and that commit is supposed to
answer the question. If the commit is huge (e.g. a squash of an entire
feature), then the question is not answered, and git-blame has been
rendered useless.


Nothing in my statement of "don't exclude squash merge workflow"
requested that we allow a PR to be squashed into a single commit that
you believe should be a patch series.
If the button is there, maintainers will click it even in cases when
they shouldn't, and I won't be able to catch them. The result will not
necessarily hurt the maintainer (not at once, anyway), but it will harm
others that investigate the git history afterwards -- possibly years
later.

I can't watch all CoreFoobarPkg pull requests on github to prevent this.
On the other hand, I can, and do, monitor the edk2-devel list for
seriously mis-organized patch sets, especially for core packages where
I've formed an "I had better watch out for this core package"
impression.

I have made requests under core patch sets where I was mostly unfamiliar
with the technical subject *for the time being*, asking just for
improvements to the granularity of the series. Knowing the improved
granularity might very well help me *in the future*.


The mailing list equivalent of "squash-on-merge" would be the following:

- contributor posts v1 with patches 1/5 .. 5/5 (for example),

- reviewer requests updates A, B, and C,

- contributor posts (in response to the v1 blurb, i.e. 0/5) further
patches 6/8, 7/8, 8/8

- reviewer checks the new patches and approves them, functionally,

- maintainer says "OK let me merge this",

- maintainer applies the patches (all 8 of them) from the list, on a
local branch,

- maintainer runs a git rebase squashing the whole thing into a single
patch,

- maintainer does *not* review the result,

- maintainer opens a PR with the resultant single patch,

- CI passes,

- the patch is merged.


With the list-based process, the disaster in the last step is mitigated
in at least three spots:

- All subscribers have a reasonably good chance to notice and intervene
when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to
the v1 blurb, clearly with an intent to squash.

- Because the maintainer has to do *extra work* for the squashing, the
natural laziness of the maintainer works *against* the disaster. Thus
he or she will likely not perform the local rebase & squash. Instead
they will ask the contributor to perform a *fine-grained* squash (i.e.
squash each fixup into the one original patch where the fixup
belongs), and to submit a v2 series.

- If someone interested in the git history catches (after the fact) that
the maintainer merged a significantly different patch set from what
had been posted and reviewed, they can raise a stern complaint on the
list, and next time the maintainer will now better.

(This is not a theoretical option; I low-key follow both the list
traffic and the new commits in the git history (whenever I pull). In the
past I had reported several patch application violations (mismanaged
feedback tags, intrusive updates post-review, etc). Nowadays it's gotten
quite OK, thankfully, and I'm terrified of losing those improvements.)


If we just plaster a huge squash-on-merge button or checkbox over the
web UI, it *will* be abused (maintainer laziness will work *towards* the
disaster), with only a microscopic chance for me to prevent the abuse.

It's not that "I believe" that this or that *particular* series should
not be squashed. "Not squashing" is not the exception but the rule. The
*default* approach is that the submitter incorporates incremental fixes
into the series at the right stages, they maintain a proper series
structure over the iterations, and they propose revised versions of the
series in full. Squashing is the exception; for example one reason is,
"if you separate these changes from each other, then the tree will not
build in the middle; they belong together, please squash them, and
resubmit for review".


I do think those rules will need to be defined but that is needed
today anyway.
Rules are only as good as their enforcement is.
In my work world we require code review by a manager and that is the de facto enforcement mechanism. Basically there is always an owner to make sure the process was followed :)

Also in our world the squash is a developer choice. But we do have tools that insert the Bugzilla number in all the commits of the series, assist with the squash, etc.

The question is not how nice it is to use squash-on-merge in the
minuscule set of situations when it might be justified; the question is
how difficult it would be to prevent the inevitable abuses.

The list lets me advocate for proper git history hygiene reasonably
efficiently (although I still miss a bunch of warts due to lack of
capacity). With the squash-on-merge button or checkbox, the flood gates
would fly open. I won't stand for that (not as a steward anyway).

I think our world views differ fundamentally. I value the git history
*way* above my own comfort, and everyone else's (accounting for both
contributor and day-to-day maintainer roles). I guess you prefer the
reciprocal of that ratio.
I'd also point out that the processes you chose kind of defines your quanta of work. It is likely you would be willing to tackle a really big change as a large patch set, that you would likely break up into multiple PRs in a squash on commit world. In a squash on commit world you also might break a Bugzilla (BZ) up into dependent BZs, a tree of BZs. That might sound crazy, but when you work on a bigger project and there are BZs for EFI, T2, macOS, the Installer, and the RecoveryOS for a customer visible feature this tree of BZ might be familiar and make sense to you.

But I think the real argument for consistency is we have a rich git history that has value. We have made resource tradeoffs to have that rich git history so to me it makes the most sense, for these project, to try to preserve our past investment in git history.

Thanks,

Andrew Fish

Thanks,
Laszlo


Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Laszlo Ersek
 

On 05/22/20 07:48, Bret Barkelew wrote:

In Mu we have a similar problem of keeping track of what features/bugs
have already been upstreamed and when can they be dropped during an
upstream integration, so that's the more personal interest I have in
such automation.
Proposal:

- Whenever upstreaming a bugfix or a feature, open an upstream BZ.

- In your downstream ticket for the same bugfix or feature,
cross-reference the upstream BZ URL. This shouldn't be a normal comment,
but a dedicated field. In Bugzilla, there is "See Also" (it can carry a
list of URLs). In our own (RH) Bugzilla instance, "See Also" has been
replaced with an "External Trackers" list, but the idea is the same.

- When you rebase, run a git-log over the upstream commit history being
straddled, and collect the upstream BZs referenced. For example:

$ git log edk2-stable201911..edk2-stable202002 \
| grep -E -o 'https://bugzilla.tianocore.org/show_bug.cgi\?id=[0-9]+' \
| sort -u

This reliably presents the set of upstream BZs that were *touched on* in
the subject development cycle, because TianoCore contributors diligently
reference BZs in commit messages. Right? :)

- Use a script to fetch the fresh status of each of those BZ URLs,
because in some cases, "touched on a BZ" does not guarantee "fixed BZ".
Some BZs may require multiple waves of patches.

Of course, BZs that *have* been fixed will all report RESOLVED|FIXED,
because TianoCore contributors and maintainers diligently close BZs as
FIXED when the corresponding patches are merged. They even mention the
commit range(s) implementing the related code changes, without fail.
Right? :)

- Once you have your set of Really Fixed (TM) upstream BZs, run a search
in your downstream tracker to locate the referring downstream tickets,
checking the "See Also" (etc) fields.


In a more serious tone: while Red Hat preaches and practices "upstream
first", we obviously *do* have downstream tickets for bugfixes and
features. And if we are *inheriting* patches for them via a rebase (as
opposed to backporting / cherry-picking them), then we benefit from the
same kind of linkage. That's why I keep "lecturing" maintainers when
they fail to close BZs, and/or to note the subject commit ranges (which
I might want to investigate manually).

Now, I realize that "git forges" can auto-close tickets when
encountering ticket references in merged patches. The problem is that
*multiple* patches may reference a ticket and *still* not constitute a
complete fix for that ticket -- see my "multiple waves of patches" note
above. Automation cannot fully supplant manual ticket wrangling.


NB, the above procedure could also help with composing the "feature
list" for any upcoming edk2 stable tag. When collecting the URLs, and
checking their fresh statuses, also check the "Product" fields. If
Product is "TianoCore Feature Requests", then the ticket is a good
candidate to name at
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#proposed-features>.

Thanks,
Laszlo

401 - 420 of 747