New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
firefox: Fix AArch64 build #81211
firefox: Fix AArch64 build #81211
Conversation
This change would break the
|
While looking at this I noticed that |
@andir does the other patch apply? I assumed it didn't for aarch64, though I could have been wrong. I assumed only because ESR is relatively old, and I assumed the same patch wouldn't apply for the range ESR→stable. I'll check. @thefloweringash this was built on the community builder. I asked about SRI hashes on #nixos-dev but had no clear answer as to whether it should be used or not. This looks like a major issue. Is it tracked? It does require a new-enough nix, 2.2 from January 2019. As far as the hash is concerned, I'll put an old basewhatever hash instead. |
4e7dd6a
to
8bd741e
Compare
I'm looking more into it, and sure, esr didn't build, patches didn't apply. My assumptions seemed true, though they were not right that I should have just left that hanging behind. |
8bd741e
to
d558406
Compare
See #79987 (review) for the SRI hash support in fetchpatch and fetchgit. |
@andir ready for review, firefox was left untouched, and firefox-esr now builds. |
This would end up being the similar change to backport, if we don't want to remove diff --git a/pkgs/applications/networking/browsers/firefox/common.nix b/pkgs/applications/networking/browsers/firefox/common.nix
index 560b01d1b52..5a487603d0d 100644
--- a/pkgs/applications/networking/browsers/firefox/common.nix
+++ b/pkgs/applications/networking/browsers/firefox/common.nix
@@ -120,6 +120,11 @@ let
sha256 = "1zg56v3lc346fkzcjjx21vjip2s9hb2xw4pvza1dsfdnhsnzppfp";
})
]
+ ++ lib.optional (lib.versionAtLeast ffversion "73") (fetchpatch {
+ # https://phabricator.services.mozilla.com/D60667
+ url = "https://hg.mozilla.org/mozilla-central/raw-rev/b3d8b08265b800165d684281d19ac845a8ff9a66";
+ sha256 = "0b4s75w7sl619rglcjmlyvyibpj2ar5cpy6pnywl1xpd9qzyb27p";
+ })
++ patches;
nss_pkg = if lib.versionAtLeast ffversion "71" then nss_3_49_2 else nss;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this! Looks good overall. Started a build (to confirm) and had one more small note.
url = "https://raw.githubusercontent.com/archlinuxarm/PKGBUILDs/09c7fa0dc1d87922e3b464c0fa084df1227fca79/extra/firefox/arm.patch"; | ||
sha256 = "1vbpih23imhv5r3g21m3m541z08n9n9j1nvmqax76bmyhn7mxp32"; | ||
}) | ||
] ++ lib.optionals (stdenv.isAarch64) ([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking: Does it harm if we always apply that patch? It would certainly mean we notice it earlier if it fails to apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't [h]arm in most cases, but I haven't reviewed the opus patch to know exactly what it does.
And you're right, I would think it's preferable to unconditionally patch, if the patch has issues on other platforms, it means it's not upstreamable as-is, and we probably shouldn't include patches that are not upstreamable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find an upstream issue for that opus patch. I'd rather not include it by default then. The other patch (https://github.com/NixOS/nixpkgs/pull/81211/files#diff-b39b9a678f9ecf5d0e1581832f7bf99dR104) was merged upstream and should probably just be included at all times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just build successfully without opus patch.
upstream info:
https://bugzilla.mozilla.org/show_bug.cgi?id=1530659
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! @samueldr can you remove the opus patch and then merge this?
* The 'arm.patch' patch doesn't apply anymore. * The 'build-arm-libopus.patch' patch isn't required anymore. * See the mozilla phabricator link for the added patch. Additionally, we are now *always* undconditionally applying all patches to all architectures. That is, unless they have undesirable side-effects, but those might not be fit for inclusion. By applying all patches all the time, they'll be removed or replaced when they stop applying.
d558406
to
d4446c5
Compare
Tested build for:
On platforms:
|
The removed patch doesn't apply anymore.
See the mozilla phabricator link for the added patch.
Motivation for this change
Make firefox build on aarch64.
The upgrade seemingly wasn't looked at all for aarch64.
On current unstable a patch simply doesn't apply.
Though, on 19.09, that same patch seemingly applies????
The added patch fixes the compilation issue in 19.09, which also happens on unstable once the patch is removed.
This will need a backport to 19.09 and 20.03. (Which I can handle.) I do not know whether the removed patch needs to be kept for 19.09 or 20.03.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)cc @andir