clang_{5,6,7}: add --sysroot argument pointing to gcc toolchain prefix #53668

Twey · 2019-01-08T20:36:46Z

Motivation for this change

Things done

matthewbauer · 2019-01-08T21:51:20Z

Can we just set sysroot to empty? Otherwise we have to be careful of not propagating a dependcy on gcc.

Twey · 2019-01-08T22:36:05Z

Non-existent directories are invalid, plus clang without this patch seems to pick up some libraries from GCC that I'm not sure shouldn't be visible. GCC is passed in anyway as --gcc-toolchain so it seems like a safe option.

LnL7 · 2019-01-08T22:38:51Z

pkgs/development/compilers/llvm/7/default.nix

@@ -23,6 +23,7 @@ let
      ln -s "${cc}/lib/clang/${release_version}/include" "$rsrc"
      ln -s "${targetLlvmLibraries.compiler-rt.out}/lib" "$rsrc/lib"
      echo "-resource-dir=$rsrc" >> $out/nix-support/cc-cflags
+      echo "--sysroot=${tools.clang-unwrapped.gcc}" >> $out/nix-support/cc-cflags


clang-unwrapped.gcc is only valid linux with a gcc compiler if it needs to point to that then it must be conditionalized properly and same with the line below actually

nixpkgs/pkgs/build-support/cc-wrapper/default.nix

Lines 47 to 48 in cd8c1a4

default_cxx_stdlib_compile = optionalString (targetPlatform.isLinux && !(cc.isGNU or false) && !nativeTools && cc ? gcc)

"-isystem $(echo -n ${cc.gcc}/include/c++/*) -isystem $(echo -n ${cc.gcc}/include/c++/*)/$(${cc.gcc}/bin/gcc -dumpmachine)";

How does this relate to the -isystem flags set in the stdenv?

-i is just for header files, and the sysroot doesn't affect paths given with absolute paths, just the search path for libraries given by name and toolchain binaries called during compilation (e.g. ld). I don't think these things interact.

I'm a bit suspicious of these flags because I think they're included even if you want to use libc++, but I think that's the topic of a separate PR.

It's the default, the variables are configured by the libc++ / libstdcxxHook hooks instead. I'm pretty sure we could actually get rid of this attribute now.

However doesn't this change have the problem you mentioned and break libcxxStdenv then? Whether libstdc++ or libc++ should be used and what version is determined by the extraPackages added to the cc-wrapper. In it's current form, if I'm not mistaken, this change would result in llvmPackages_7.stdenv using a --sysroot that points to llvmPackages_5.clang-unwrapped (default stdenv on darwin) which doesn't seem right to me.

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

Line 54 in ec3e81b

libstdcxxHook

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

Lines 63 to 64 in ec3e81b

targetLlvmLibraries.libcxx

targetLlvmLibraries.libcxxabi

I've tested this (on Linux) and I don't believe it breaks libcxxStdlib. It puts libstdc++ on the library search path to be sure, but ultimately which stdlib is linked in is controlled by -stdlib as it's always been.

I'm less sure about Darwin and I don't have a system to test on, but on Linux llvmPackages_7.stdenv still gets the gcc sysroot. Does Darwin use libcxxStdenv by default? If that's so then I think it's also fine. I think the default sysroot is /, so this shouldn't break anything pure — in the worst case it adds the gcc libraries (including libstdc++) to the library search path, which are not necessary because they're not going to be linked in. But I don't think anything even gains gcc as an input that didn't have it before — if the toolchain isGNU then cc refers to a gcc that's already pulled in as clang-unwrapped.gcc, and otherwise it refers to clang itself I guess, which is fine since we're not going to be relying on libstdc++.

Currently libstdc++ is given by the sysroot; if we want to change that behaviour maybe it should go in a separate PR?

Twey · 2019-01-09T10:45:48Z

Specifically it needs libstdc++.a to successfully compile C++ unless you're using libc++. I've changed it to use stdenv.cc.cc, though, which I believe is safe across platforms?

Twey · 2019-01-24T16:57:34Z

Is this okay? We're depending on this patch in-house (we use Clang on non-NixOS systems, and we need it to not grab system libraries).

LnL7 · 2019-01-24T22:43:27Z

I don't believe it is for a clang based systems, but I might be wrong since I don't fully understand what that flag is supposed to do. Why does this need to point to gcc instead of clang itself?

EDIT:

output '/nix/store/2866q70dfvg3x6ici9sgw8vsinqrzhvl-stdenv-darwin' is not allowed to refer to the following paths:
     /nix/store/2ws9cmamvr7xyvdg4d2nnd1bmr1zjrrq-bootstrap-tools

matthewbauer · 2019-01-24T22:58:37Z

What the closure impact for this? I would much prefer this being --sysroot=/homeless-shelter or something like that.

TBH I don't understand why this is an issue. Don't you want your compiler to look in all of the directories for libraries? They shouldn't be given priority over LDFLAGS, but they definitely should be searched.

LnL7 · 2019-01-24T23:02:41Z

That makes me even more confused, it shouldn't be pointing to the stdenv compiler at all in that case. If the path must exist $out or ${cc.cc} would make much more sense to me.

Twey · 2019-01-28T13:45:44Z

@matthewbauer Non-existent directories are invalid. We can make an empty derivation or something to point to — or point it to clang itself — but then we need to make further modifications to make sure all the gcc libraries (e.g. libstdc++) are found, which is currently done implicitly through the sysroot.

We don't want the compiler to be looking in global directories like /lib and /usr/lib, because this is impure. It means that, for example, nix-shell --pure or builds without sandboxing can exhibit impure behaviour where they succeed/fail based on external system libraries, which is a big issue for a reproducible build system.

Twey · 2019-01-28T13:47:38Z

As a concrete example, I ran across this issue when a Meson-based build that worked fine on my NixOS system failed on my coworker's Debian system, because he happened to have /usr/lib/libfoo.a and Meson uses the Clang library search paths to find and replace libraries in the linker command line.

matthewbauer · 2019-02-12T00:50:06Z

The issue with this is clang forms its own stdenv in many cases. This is by default on Darwin and is also used frequently on linux through clangStdenv. We don't want the GCC part leaking through in this case. It sounds like Meson is doing something really weird here though.

Twey · 2019-02-12T13:14:33Z

I agree it's weird, but Meson relies heavily on this behaviour and Clang shouldn't be giving impure paths in response to requests anyway — we should patch this out as we patch out other impure references in packages. Would you rather we had a patch to the source? This issue is currently blocking upgrade of nixpkgs at my company.

matthewbauer · 2019-02-15T15:53:34Z

pkgs/development/compilers/llvm/5/default.nix

@@ -23,6 +23,7 @@ let
      ln -s "${cc}/lib/clang/${release_version}/include" "$rsrc"
      ln -s "${targetLlvmLibraries.compiler-rt.out}/lib" "$rsrc/lib"
      echo "-resource-dir=$rsrc" >> $out/nix-support/cc-cflags
+      echo "--sysroot=${stdenv.cc.cc}" >> $out/nix-support/cc-cflags


Maybe this can work. But i think it should be cc not stdenv.cc.cc so that it comes from clang.

Long-term I'd rather patch out the compiled-in search paths altogether and figure out a better way to switch between C++ stdlibs. As stands, though, this patch is designed to preserve the existing behaviour of finding gcc via the sysroot and using that to pick up libstdc++. Setting the sysroot to (clang) cc, or indeed any valid path, will fix the purity issue, but breaks any builds that expect clang++ to work out of the box — all C++ programs will need to explicitly include their stdlib as a dependency. Currently libstdc++ is found by --gcc-toolchain, but setting --sysroot overrides that, so we just put it back by explicitly setting the sysroot to gcc.

Hmm... The wrapper should have handling for this through default_cxx_stdlib_compile. Is that maybe not working as expected:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/cc-wrapper/default.nix#L48

My concern with using stdenv here is it pull in the parent's stdenv which could mess up things like cross compilation. But I definitely want to find a way to get rid of the --gcc-toolchain thing in the future, so perhaps we can handle them together.

matthewbauer

I'm okay with merging this now, as the changes seem like a step in the right direction. Two things are still needed though:

either base off staging and test on darwin or make conditional on linux.
add llvm 7

stale · 2020-12-25T07:22:08Z

I marked this as stale due to inactivity. → More info

wegank · 2024-03-20T13:30:49Z

Closed by the removal of LLVM <= 7, please open a new PR targeting higher versions if still relevant.

Twey requested a review from matthewbauer as a code owner January 8, 2019 20:36

LnL7 reviewed Jan 8, 2019

View reviewed changes

Twey force-pushed the clang-purity branch from d7cb3a6 to d493473 Compare January 9, 2019 10:44

GrahamcOfBorg added 10.rebuild-darwin-stdenv 10.rebuild-darwin: 501+ 10.rebuild-linux: 11-100 labels Jan 9, 2019

matthewbauer closed this Feb 12, 2019

matthewbauer reopened this Feb 15, 2019

matthewbauer requested changes Feb 15, 2019

View reviewed changes

GrahamcOfBorg added the 10.rebuild-darwin: 5001+ label Feb 15, 2019

Twey force-pushed the clang-purity branch 2 times, most recently from 85a98d8 to 8e85822 Compare February 20, 2019 14:38

matthewbauer approved these changes Aug 1, 2019

View reviewed changes

clang_{5,6,7}: add --sysroot argument pointing to gcc toolchain prefix

77f454e

Twey force-pushed the clang-purity branch from 8e85822 to 77f454e Compare August 23, 2019 10:07

ofborg bot added 10.rebuild-linux: 101-500 and removed 10.rebuild-linux: 11-100 labels Aug 23, 2019

FRidh requested a review from LnL7 November 3, 2019 13:56

FRidh changed the base branch from master to staging November 3, 2019 13:57

FRidh requested a review from matthewbauer November 3, 2019 13:57

nh2 mentioned this pull request Feb 19, 2020

Using clang is impure #24697

Open

matthewbauer mentioned this pull request Mar 23, 2020

Compiling bytes: fatal error: 'bits/libc-header-start.h' file not found reflex-frp/reflex-platform#645

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020

NixOS deleted a comment from stale bot Jun 26, 2020

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 26, 2020

matthewbauer mentioned this pull request Jun 26, 2020

Can't build with alternate compilers (Clang) on Ubuntu 18.04 #91561

Open

veprbl added the 2.status: merge conflict label Jun 27, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 25, 2020

veprbl added this to WIP in Staging via automation Nov 14, 2021

Artturin added the 12.approvals: 1 label May 4, 2022

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 4, 2022

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 2, 2022

wegank removed the 12.approvals: 1 label Sep 7, 2023

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 7, 2023

wegank marked this pull request as draft January 5, 2024 23:25

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024

wegank closed this Mar 20, 2024

Staging automation moved this from WIP to Done Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clang_{5,6,7}: add --sysroot argument pointing to gcc toolchain prefix #53668

clang_{5,6,7}: add --sysroot argument pointing to gcc toolchain prefix #53668

Twey commented Jan 8, 2019 •

edited

matthewbauer commented Jan 8, 2019

Twey commented Jan 8, 2019 •

edited

LnL7 Jan 8, 2019 •

edited

Twey Jan 9, 2019

Twey Jan 9, 2019

LnL7 Jan 9, 2019

Twey Jan 11, 2019 •

edited

Twey Jan 11, 2019

Twey commented Jan 9, 2019

Twey commented Jan 24, 2019

LnL7 commented Jan 24, 2019 •

edited

matthewbauer commented Jan 24, 2019 •

edited

LnL7 commented Jan 24, 2019

Twey commented Jan 28, 2019

Twey commented Jan 28, 2019

matthewbauer commented Feb 12, 2019

Twey commented Feb 12, 2019

matthewbauer Feb 15, 2019

Twey Feb 16, 2019

matthewbauer Aug 1, 2019

matthewbauer left a comment

stale bot commented Dec 25, 2020

wegank commented Mar 20, 2024

	default_cxx_stdlib_compile = optionalString (targetPlatform.isLinux && !(cc.isGNU or false) && !nativeTools && cc ? gcc)
	"-isystem $(echo -n ${cc.gcc}/include/c++/) -isystem $(echo -n ${cc.gcc}/include/c++/)/$(${cc.gcc}/bin/gcc -dumpmachine)";

clang_{5,6,7}: add --sysroot argument pointing to gcc toolchain prefix #53668

clang_{5,6,7}: add --sysroot argument pointing to gcc toolchain prefix #53668

Conversation

Twey commented Jan 8, 2019 • edited

Motivation for this change

Things done

matthewbauer commented Jan 8, 2019

Twey commented Jan 8, 2019 • edited

LnL7 Jan 8, 2019 • edited

Choose a reason for hiding this comment

Twey Jan 9, 2019

Choose a reason for hiding this comment

Twey Jan 9, 2019

Choose a reason for hiding this comment

LnL7 Jan 9, 2019

Choose a reason for hiding this comment

Twey Jan 11, 2019 • edited

Choose a reason for hiding this comment

Twey Jan 11, 2019

Choose a reason for hiding this comment

Twey commented Jan 9, 2019

Twey commented Jan 24, 2019

LnL7 commented Jan 24, 2019 • edited

matthewbauer commented Jan 24, 2019 • edited

LnL7 commented Jan 24, 2019

Twey commented Jan 28, 2019

Twey commented Jan 28, 2019

matthewbauer commented Feb 12, 2019

Twey commented Feb 12, 2019

matthewbauer Feb 15, 2019

Choose a reason for hiding this comment

Twey Feb 16, 2019

Choose a reason for hiding this comment

matthewbauer Aug 1, 2019

Choose a reason for hiding this comment

matthewbauer left a comment

Choose a reason for hiding this comment

stale bot commented Dec 25, 2020

wegank commented Mar 20, 2024

Twey commented Jan 8, 2019 •

edited

Twey commented Jan 8, 2019 •

edited

LnL7 Jan 8, 2019 •

edited

Twey Jan 11, 2019 •

edited

LnL7 commented Jan 24, 2019 •

edited

matthewbauer commented Jan 24, 2019 •

edited