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

llvmPackages: improve gcc compatibility #330316

Merged
merged 2 commits into from
Aug 4, 2024

Conversation

RossComputerGuy
Copy link
Member

@RossComputerGuy RossComputerGuy commented Jul 27, 2024

Description of changes

As recommended by @tomberek, add common flags to prevent many issues that LLVM would have but GCC wouldn't have.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label Jul 27, 2024
@RossComputerGuy RossComputerGuy changed the title llvmPackages.clangUseLLVM: add common flags for gcc compatibility llvmPackages: improve gcc compatibility Jul 27, 2024
@RossComputerGuy RossComputerGuy force-pushed the feat/llvm-common-opts branch from c82ac64 to a9d38f5 Compare July 27, 2024 04:40
@@ -157,6 +158,11 @@ stdenv.mkDerivation (rec {
end
MRI
fi
''
+ lib.optionalString doFakeLibstdcxx ''
ln -s $out/lib/libc++.so $out/lib/libstdc++.so
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need the libstdc++.so.6 variants?

Copy link
Contributor

@paparodeo paparodeo Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you could get around this perhaps by adding a flag when building clang using -DCLANG_DEFAULT_CXX_STDLIB=libc++ for pkgsLLVM so clang -lstdc++ will get translated to clang -lc++. this is the default on darwin but not on linux.

@RossComputerGuy RossComputerGuy force-pushed the feat/llvm-common-opts branch from a9d38f5 to 5f0f0c2 Compare July 31, 2024 02:51
@RossComputerGuy RossComputerGuy marked this pull request as ready for review July 31, 2024 05:04
Majority of packages with a version script have broken version scripts,
this will likely result in us adding this flag to many packages. To keep
things simple, just add it as a default.
@RossComputerGuy RossComputerGuy force-pushed the feat/llvm-common-opts branch from 5f0f0c2 to 019d091 Compare July 31, 2024 14:08
@tomberek tomberek merged commit ad0f21f into NixOS:master Aug 4, 2024
23 checks passed
@RossComputerGuy RossComputerGuy deleted the feat/llvm-common-opts branch August 4, 2024 05:38
@spiage
Copy link

spiage commented Aug 4, 2024

This commit breaks firefox and thunderbird

error: builder for '/nix/store/slax6bvi346jmqjpbh3ixizjgs4bhir5-thunderbird-unwrapped-115.13.0.drv' failed with exit code 1;
last 10 log lines:
> 0:09.06(B DEBUG: | ;(B(B
> 0:09.06(B DEBUG: | return 0;(B(B
> 0:09.06(B DEBUG: | }(B(B
> 0:09.06(B DEBUG: Executing: /nix/store/jl7pn7ahvq0kyr86pf0w2c2h9ca1pn94-wasm32-unknown-wasi-clang-wrapper-18.1.8/bin/wasm32-unknown-wasi-cc -std=gnu99 /build/conftest.4mv9xe3w.c --sysroot=/nix/store/6mqf2cb15b436zgwc84dgjpv5vbawih0-wasi-sysroot(B(B
> 0:09.06(B DEBUG: The command returned non-zero exit status 1.(B(B
> 0:09.06(B DEBUG: Its error output was:(B(B
> 0:09.06(B DEBUG: | wasm-ld: error: unknown argument: --undefined-version(B(B
> 0:09.06(B DEBUG: | clang: error: linker command failed with exit code 1 (use -v to see invocation)(B(B
> 0:09.06(B ERROR: Cannot find wasi libraries or problem with the wasm linker. Please fix the problem. Or build with --without-wasm-sandboxed-libraries.(B(B
> *** Fix above errors and then restart with "./mach build"
For full logs, run 'nix log /nix/store/slax6bvi346jmqjpbh3ixizjgs4bhir5-thunderbird-unwrapped-115.13.0.drv'.
error: 1 dependencies of derivation '/nix/store/cv3r6hwbxhrkq50f060q3c4ajqb2dmfv-thunderbird-115.13.0.drv' failed to build
note: keeping build directory '/tmp/nix-build-firefox-unwrapped-128.0.3.drv-2/build'
error: builder for '/nix/store/4asrnhz2i3whvapghmalk1cpjsn3rja1-firefox-unwrapped-128.0.3.drv' failed with exit code 1;
last 10 log lines:
> 0:10.50(B DEBUG: | ;(B(B
> 0:10.50(B DEBUG: | return 0;(B(B
> 0:10.50(B DEBUG: | }(B(B
> 0:10.50(B DEBUG: Executing: /nix/store/jl7pn7ahvq0kyr86pf0w2c2h9ca1pn94-wasm32-unknown-wasi-clang-wrapper-18.1.8/bin/wasm32-unknown-wasi-cc -std=gnu99 /build/conftest.muc6eb2g.c --sysroot=/nix/store/6mqf2cb15b436zgwc84dgjpv5vbawih0-wasi-sysroot(B(B
> 0:10.50(B DEBUG: The command returned non-zero exit status 1.(B(B
> 0:10.50(B DEBUG: Its error output was:(B(B
> 0:10.50(B DEBUG: | wasm-ld: error: unknown argument: --undefined-version(B(B
> 0:10.50(B DEBUG: | clang: error: linker command failed with exit code 1 (use -v to see invocation)(B(B
> 0:10.50(B ERROR: Cannot find wasi libraries or problem with the wasm linker. Please fix the problem. Or build with --without-wasm-sandboxed-libraries.(B(B
> *** Fix above errors and then restart with "./mach build"
For full logs, run 'nix log /nix/store/4asrnhz2i3whvapghmalk1cpjsn3rja1-firefox-unwrapped-128.0.3.drv'.

@RossComputerGuy
Copy link
Member Author

Sorry about that @spiage, #332170 should fix it.

@spiage
Copy link

spiage commented Aug 4, 2024

Thank you! =)
I'll tell you the result if you want

@alyssais
Copy link
Member

alyssais commented Aug 5, 2024

Can you add me as a reviewer on PRs like this in future? I keep not finding out about them until after they've already been merged, despite being an LLVM maintainer.

I'm not convinced this is a good idea — compiler authors turn things into errors for a reason, and as a distribution we have our part to play in getting problematic things fixed. It's fine to disable the errors for packages where it's not reasonably possible to get them fixed, but doing it globally means even packages that would be simple to fix properly don't get fixed.

@alyssais
Copy link
Member

alyssais commented Aug 5, 2024

To expand a bit, the problem with this sort of thing is that it's very difficult later to tell when it can be removed. By setting this globally instead of setting it for individual packages that need it, we're effectively stuck with it forever, because it's impossible to tell if removing it is going to break any packages. If we mark packages individually, it's very easy to tell in future when those markings can be removed.

This isn't a theoretical concern: something similar was done for pkgsMusl — some compatibility headers were added globally, instead of fixing individual affected packages, and now, even though we've discovered that the presence of those headers actually causes problems for other packages, it's impossible to know what the fallout would be of removing it. Let's not repeat this mistake.

@timothyklim
Copy link
Contributor

timothyklim commented Oct 5, 2024

Now I get ld: unknown option: --undefined-version when I switched to nixos-unstable for macOS and using llvmPackages_17.

@timothyklim
Copy link
Contributor

Now I get ld: unknown option: --undefined-version when I switched to nixos-unstable for macOS and using llvmPackages_17.

In nixos-24.05 everything works.

@timothyklim
Copy link
Contributor

@RossComputerGuy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants