[staging-next] electron: fix build#318857
Conversation
I'm pretty sure this was a mistake — in Nixpkgs the target platform is the platform that the program being built should output executables for — i.e., it's only relevant for a compiler, which Chromium is not. Tested that cross-compilation of Electron still works.
Just like with Firefox, we need to make sure there's only a single version of LLVM involved in building Chromium, or we get errors like this: ld.lld: error: Invalid record (Producer: 'LLVM18.1.7' Reader: 'LLVM 17.0.6') Fixes: 23d4f83 ("cargo,clippy,rustc,rustfmt: 1.77.2 -> 1.78.0")
ofborg
bot
added
10.rebuild-darwin: 0
doronbehar
left a comment
doronbehar
left a comment
There was a problem hiding this comment.
Diff looks very good. Should be good to go if CI is greener.
emilylange
left a comment
emilylange
left a comment
There was a problem hiding this comment.
I think there is a stray pkgsBuildTarget in chromium/default.nix now:
diff --git a/pkgs/applications/networking/browsers/chromium/default.nix b/pkgs/applications/networking/browsers/chromium/default.nix
index 317cb7de1011..9ce37e18fa86 100644
--- a/pkgs/applications/networking/browsers/chromium/default.nix
+++ b/pkgs/applications/networking/browsers/chromium/default.nix
@@ -18,7 +18,6 @@
, cupsSupport ? true
, pulseSupport ? config.pulseaudio or stdenv.isLinux
, commandLineArgs ? ""
-, pkgsBuildTarget
, pkgsBuildBuild
, pkgs
}:But besides that, I don't feel confident reviewing this.
The code in question is mostly cross-compilation stuff that Amjoseph did before I joined chromium.meta.maintainers and I barely know anything about cross-compilation.
In case it helps other reviewers, the commits that introduced pkgsBuildTarget and llvmPackages_attrName were part of #229265, specifically c25897c 5f3c644.
Taking llvm* from rustc makes sense to me though, for what it's worth.
Thanks :)
wegank
added
the
12.approvals: 1
Looks like it's been stray since 5f3c644. I'll remove it. |
|
I confirm |
|
|
|
FYI, on some versions electron builds are still regressing (since nixpkgs master): |
|
Can do the backporting, if you want me to. That should unblock it. But please be aware that Edit: see https://www.electronjs.org/docs/latest/tutorial/electron-timelines#timeline |
|
FYI, on some versions electron builds are still regressing (since nixpkgs master):
https://hydra.nixos.org/eval/1806937?filter=electron&compare=1806929#tabs-now-fail
Some patches probably need to be backported to get it to build with LLVM 18,
e.g. https://webrtc.googlesource.com/src/+/267f9bdd53a37d1cbee760d5af07880198e1beef%5E%21/
|
|
Ah, so maybe we should just set knownVulnerabilities, and then it'll be irrelevant for Hydra anyway, but people who need old Electron can fix it if they want to? |
|
Yes, I don't think maintenance is really expected on EOL versions. |
|
Quite a few packages depend on these two, so we might e.g. notify their meta.maintainers I guess. |
Link: https://www.electronjs.org/docs/latest/tutorial/electron-timelines#timeline Link: NixOS#318857 (comment) (cherry picked from commit 7892638)
5 participants
Description of changes
Things done
Just like with Firefox, we need to make sure there's only a single
version of LLVM involved in building Chromium, or we get errors like
this:
Tested that cross compilation of Electron still works.
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.