Skip to content
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

Merged
merged 1 commit into from Mar 9, 2020
Merged

Conversation

samueldr
Copy link
Member

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @andir

@samueldr samueldr added the 9.needs: port to stable A PR needs a backport to the stable release. label Feb 27, 2020
@samueldr samueldr requested a review from andir February 27, 2020 21:36
@ofborg ofborg bot requested a review from edolstra February 27, 2020 21:51
@andir
Copy link
Member

andir commented Feb 28, 2020

This change would break the firefox-esr package. Perhaps we must retain the old patch in case firefox is sufficiently old (68.X.Yesr) and use the newer patch for all the other builds?

npacking source archive /nix/store/222za5548bfrhrpfm13g1dpng75xhsiy-firefox-68.5.0esr.source.tar.xz
source root is firefox-68.5.0
setting SOURCE_DATE_EPOCH to timestamp 1581035317 of file firefox-68.5.0/sourcestamp.txt
patching sources
applying patch /nix/store/z0ib43s19d95ljf77cz7dmg86zi86f95-env_var_for_system_dir.patch
patching file toolkit/xre/nsXREDirProvider.cpp
Hunk #1 succeeded at 305 (offset 3 lines).
applying patch /nix/store/5r8vjl15hvxnwn1hq7px0grakz308lv7-build-arm-libopus.patch
patching file media/libopus/silk/arm/arm_silk_map.c
applying patch /nix/store/vcnf3qq5v97l3grjcj4cw4nalmi7w0x8-b3d8b08265b800165d684281d19ac845a8ff9a66
patching file gfx/2d/SwizzleNEON.cpp
Hunk #1 FAILED at 407.
1 out of 1 hunk FAILED -- saving rejects to file gfx/2d/SwizzleNEON.cpp.rej
builder for '/nix/store/2f3876vcid3vbla44xjh7mszqs45mfzj-firefox-esr-unwrapped-68.5.0esr.drv' failed with exit code 1

@thefloweringash
Copy link
Member

While looking at this I noticed that fetchpatch can't actually fetch this patch file due to the SRI hash having a / in it. fetchpatch uses tmpfile="$TMPDIR/${args.sha256}" in postFetch, which expands to /build/sha256-94jlP07t9kC5t9f4y0pWQt4V/fa0SkZfTsFQfXg5miw=, which cannot be opened since the directory /build/sha256-94jlP07t9kC5t9f4y0pWQt4V doesn't exist.

@samueldr
Copy link
Member Author

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

@samueldr samueldr force-pushed the aarch64/firefox-73 branch 2 times, most recently from 4e7dd6a to 8bd741e Compare February 28, 2020 17:57
@samueldr
Copy link
Member Author

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.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 28, 2020

See #79987 (review) for the SRI hash support in fetchpatch and fetchgit.

@samueldr
Copy link
Member Author

@andir ready for review, firefox was left untouched, and firefox-esr now builds.

@samueldr
Copy link
Member Author

samueldr commented Mar 4, 2020

arm.patch from archlinuxarm still works for 19.09. (firefox-esr built successfully on 19.09) No. It's guarded behind versions. It's not applied. This explains the difference.

This would end up being the similar change to backport, if we don't want to remove arm.patch.

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;

Copy link
Member

@andir andir left a 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) ([
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member

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.
@samueldr
Copy link
Member Author

samueldr commented Mar 9, 2020

Tested build for:

  • firefox
  • firefox-esr

On platforms:

  • aarch64-linux
  • x86_64-linux

@samueldr samueldr merged commit b97fce7 into NixOS:master Mar 9, 2020
@samueldr samueldr deleted the aarch64/firefox-73 branch March 9, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants