edk2: backport OpenSSL 1.1.1t to the tree #244727
Conversation
|
Please test well this PR. |
|
Result of 8 packages built:
|
|
|
|
@ofborg build nixosTests.boot.uefiCdrom |
|
Should we add that to passthru.tests for edk2 (in another PR)? |
|
Please do not merge yet
I'm planning to host the patch on webarchive and fetch it from there.
Le lun. 24 juil. 2023 à 02:39, Luna Nova ***@***.***> a
écrit :
… ***@***.**** approved this pull request.
Test that broke with the previous PR passed locally and on ofborg (except
aarch64-darwin which is still queued)
Change looks reasonable, if a bit janky
—
Reply to this email directly, view it on GitHub
<#244727 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACMZRBFG4VI25ZMK7ERT53XRW75JANCNFSM6AAAAAA2TLWD7Q>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
RaitoBezarius
force-pushed
the
edk2-openssl-security-2305
branch
from
f34c4b9
to
493509d
Compare
|
The patch has been moved into webarchive and fetched from there. |
|
@ofborg build nixosTests.boot.uefiCdrom |
| # This patch has been reflowed for nixpkgs usage. | ||
| # It drops submodule modification. | ||
| # As the patch is too big for in-tree inclusion, we decided to web archive it | ||
| # and fetch it. |
There was a problem hiding this comment.
Honestly if the patch is nixpkgs-specific then even if it's big it needs to go in-tree, for lots of reasons.
If it absolutely can't go in-tree then it should go on tarballs.nixos.org.
fetchpatch FODs hash the normalized patch, not the thing that was fetched by curl... it's very much not an archival mechanism. If archive.org decides to start mangling this patch, or adds banner ads to it or something silly like that, or just deletes it, we're going to have a nightmare of a time reproducing what went on here.
It also doesn't look so good that the same person is (a) the nixpkgs UEFI expert (b) the author of this PR (c) the author of the patch (d) not hosting said patch themselves.
There was a problem hiding this comment.
Honestly if the patch is nixpkgs-specific then even if it's big it needs to go in-tree, for lots of reasons.
I just noticed that this PR is only for the release-23.05 branch.
What's wrong with putting the diff in-tree in that case? It won't become part of the long-term mainline nixpkgs, so it won't take up space for the rest of eternity.
There was a problem hiding this comment.
It also doesn't look so good that the same person is (a) the nixpkgs UEFI expert (b) the author of this PR (c) the author of the patch (d) not hosting said patch themselves.
I would love if you would not do insinuations and be clear about what you would like out of it.
We have been going through some hoops to coordinate in some channels you were not, this PR may lack of context, but I also lack a lot of time, anyone is free to adopt this PR to expedite in a better fashion. I really do not appreciate going on me like this, especially with insinuations.
Honestly if the patch is nixpkgs-specific then even if it's big it needs to go in-tree, for lots of reasons.
I would prefer this too and this was my first choice, after discussion, it was suggested to move it into webarchive like we do it for many other things in-tree, use grep to discover this.
If it absolutely can't go in-tree then it should go on tarballs.nixos.org.
fetchpatch FODs hash the normalized patch, not the thing that was fetched by curl... it's very much not an archival mechanism. If archive.org decides to start mangling this patch, or adds banner ads to it or something silly like that, or just deletes it, we're going to have a nightmare of a time reproducing what went on here.
Sure, but please take this matter in policy land, we are already using this and this is already a risk, plus, we are supposedly collecting via the find-tarballs script all the FODs and archiving them in tarballs.nixos.org, therefore, even this patch will end up there…
There was a problem hiding this comment.
Plus, you say in the review comment:
Why can't we use the patch that (according to upstream's bugtracker) "was merged"?
As explained in the Nix file: https://github.com/NixOS/nixpkgs/pull/244727/files#diff-08eafc326951de5ee5ca50ad84a9b7b0668560086eb24c328dd270bea4db9bf4R57-R59
We cannot reuse as-is the patch, if you look at the delta, you will see that I only drop the .gitmodules part because fetchFromGitHub has no support for patching the git submodules section and applying this change while downloading the submodules, if this is not clear why this is non-trivial, I can make it clearer.
There was a problem hiding this comment.
And honestly if you want me to host the patch on my personal server, I can do it, I just don't think it's something we use to do and you will probably have to convince other people than me.
There was a problem hiding this comment.
I will let you handle the magic :)
There was a problem hiding this comment.
And,
Sorry, I did not mean to insinuate anything.
Let me be totally explicit: I am not worried about this patch. What I am worried about is how it looks to outsiders. Also I am worried about other less-trustworthy people imitating this and pointing at this PR and saying "look it's okay @RaitoBezarius did it" and then I have to explain to them that I don't trust them as much as I trust you.
Thank you, you are totally right on that.
There was a problem hiding this comment.
My intuition is that excludes requires a/something or b/something, I can do it via filterdiff, but I cannot reproduce it with excludes because I don't know what the normalized patch look like.
There was a problem hiding this comment.
Thanks to a @ncfavier tip, I discovered I just needed to bust the SRI hash to get the exclusion right because of the classical phenomena.
There was a problem hiding this comment.
Thanks to a @ncfavier tip, I discovered I just needed to bust the SRI hash to get the exclusion right because of the classical phenomena.
Yeah that drives me nuts. The fix is #188587 but it needs Nix>=2.12 and impure-derivations, which require ca-derivations, which is an irreversible upgrade to /nix/store, so it's going to be a while.
RaitoBezarius
force-pushed
the
edk2-openssl-security-2305
branch
from
493509d
to
a849f8f
Compare
RaitoBezarius
force-pushed
the
edk2-openssl-security-2305
branch
2 times, most recently
from
64cda1d
to
db6dda5
Compare
Original bug: https://bugzilla.tianocore.org/show_bug.cgi?id=4342 Note that we use `excludes` here because EDK2 vendors OpenSSL via git submodules, we unbundle it, refetch it ourselves and apply in `postPatch`. Therefore, we also need to unpatch the `CryptoPkg/Library/OpensslLib/openssl`. Instead of upgrading EDK2, we decided to backport the patch manually because upgrading caused breakages in 23.05.
RaitoBezarius
force-pushed
the
edk2-openssl-security-2305
branch
from
db6dda5
to
3ed8d9b
Compare
Description of changes
Original bug: https://bugzilla.tianocore.org/show_bug.cgi?id=4342
We decide to backport this way because we do not have a lot of choices here, upgrading will break 23.05.
Prior art: #242473 (comment).
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)