[staging-next] firefox: take LLVM tools from buildStdenv

[sternenseemann]

Since 37194a3 llvmPackages*.bintools is a bintools-wrapper. Thus it
only contains a wrapper for as and ld. This change makes sense, but
causes regressions like this one. Since we need the LLVM tools
specifically opting to use llvmPackages.llvm instead of the alternative
which would be using the passed through “raw” bintools derivation
llvmPackages.bintools.bintools.

---

Untested, but verified all paths in firefox-unwrapped.makeFlags are now correct.

Motivation for this change

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/)
• Added a release notes entry if the change is major or breaking
• Fits CONTRIBUTING.md.

[vcunat]

Off the top of your head, do you understand some of the other llvm parts in firefox?

{
  # Force the use of lld and other llvm tools for LTO
  llvmPackages = llvmPackages0.override {
    bootBintoolsNoLibc = null;
    bootBintools = null;
  };
}

[sternenseemann]

That snippet forces clangUseLLVM to use the llvm bintools instead of whatever stdenv.cc.bintools is (which usually would be GNU binutils). This ensures that ${buildStdenv.cc.bintools}/bin/ld is a linker that actually can deal with clang's LTO.

Note that instead of llvmPackages.llvm in my change I could've used llvmPackages.bintools-unwrapped, buildStdenv.cc.bintools.bintools or even buildStdenv.cc.libllvm.out.

Since we have a common llvmPackages for all of those it doesn't really matter, but it may be cleaner to take the bintools from buildStdenv actually. Might amend this later, gonna eat first!

[sternenseemann]

For reference, this is how the bintools used for clangUseLLVM is chosen: If null is passed as bootBintools, then we use the LLVM bintools including lld as linker, otherwise whatever is passed in from the outside.

nixpkgs/pkgs/development/compilers/llvm/11/default.nix

Lines 52 to 59 in acb7a60

	bintoolsNoLibc' =
	if bootBintoolsNoLibc == null
	then tools.bintoolsNoLibc
	else bootBintoolsNoLibc;
	bintools' =
	if bootBintools == null
	then tools.bintools
	else bootBintools;

The default is pkgs.bintools{,NoLibc} which I don't think will differ from stdenv.cc.bintools usually.

nixpkgs/pkgs/top-level/all-packages.nix

Lines 12738 to 12767 in acb7a60

	# Here we select the default bintools implementations to be used. Note when
	# cross compiling these are used not for this stage but the *next* stage.
	# That is why we choose using this stage's target platform / next stage's
	# host platform.
	#
	# Because this is the *next* stages choice, it's a bit non-modular to put
	# here. In theory, bootstraping is supposed to not be a chain but at tree,
	# where each stage supports many "successor" stages, like multiple possible
	# futures. We don't have a better alternative, but with this downside in
	# mind, please be judicious when using this attribute. E.g. for building
	# things in *this* stage you should use probably `stdenv.cc.bintools` (from a
	# default or alternate `stdenv`), at build time, and try not to "force" a
	# specific bintools at runtime at all.
	#
	# In other words, try to only use this in wrappers, and only use those
	# wrappers from the next stage.
	bintools-unwrapped = let
	inherit (stdenv.targetPlatform) linker;
	in if linker == "lld" then llvmPackages.bintools-unwrapped
	else if linker == "cctools" then darwin.binutils-unwrapped
	else if linker == "bfd" then binutils-unwrapped
	else if linker == "gold" then binutils-unwrapped
	else null;
	bintoolsNoLibc = wrapBintoolsWith {
	bintools = bintools-unwrapped;
	libc = preLibcCrossHeaders;
	};
	bintools = wrapBintoolsWith {
	bintools = bintools-unwrapped;
	};

The intention behind these changes is to make useLLVM more flexible: You can now either have useLLVM with lld like it used to be or use other linkers (cctools, bfd, …).

[sternenseemann]

Since we are using the correct binutils actually, we should probably just set AR=llvm-ar etc. since they are correctly added to PATH…

Would make such a change on staging though since I'm not sure if there may be any sideeffects. This seems like the safer fix.

[sternenseemann]

Okay using buildStdenv.cc.bintools.bintools now — seems nicer overall.

[S-NA]

Not exactly relevant to this PR but for what it is worth, the buildStdenv abstraction exists because Firefox used to be built with GCC (and still can be if you disable LTO). Considering upstream only has CI testing on LLVM it would be best if we could move forward to removing that abstraction and just always have Firefox built with the LLVM stdenv. LTO or not.

Since we are using the correct binutils actually, we should probably just set AR=llvm-ar etc. since they are correctly added to PATH…

I am not so sure if the conditional makeFlags behind ltoSupport are still needed or if Mozilla has fixed their build system to resolve them. I have not tested it but you likely can just remove them entirely and Firefox should still build. Which I am in favor of to make the Nix expression more concise.

[sternenseemann]

You bring up valid points, but I think this is the lowest-impact change we can make which has the least chance of breaking it in a different way which is what I am looking for in a staging-next PR. I'd say we do the other changes on master next and maybe backport them to 21.05.

Confirmed this fixes the build on x86_64-linux at least:

$ nix-store -r /nix/store/yy5r4b1xk5v6x4lq3gnvc5ahby5r1wva-firefox-88.0.1.drv
/nix/store/xcxhfncn3rbk2qj7jxcsg3cgz8pzgmbz-firefox-88.0.1

[jonringer]

was about to merge :). Anyway, thanks!