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.clang: move add-nostdlibinc-flag.patch to cc-wrapper #356162

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

pwaller
Copy link
Contributor

@pwaller pwaller commented Nov 15, 2024

Remove a patch to decrease the divergence from upstream.

Motivation is a continuation of trying to reduce the number of clang
builds floating around. Wrapper builds are cheap, compiler builds are
not.

It's expected that this patch can be removed at some point and replaced
with some other cc-wrapper logic. This will be a little bit easier with
the logic already in the cc-wrapper.

Ongoing discussion in #191152. It would be nice to drop this but we
think it's probably needed at least on systems where /usr/include may be
available to prevent the possibility of picking up unwanted includes.

One potential method for eliminating nostdlibinc in the future is by
providing a non-existent -sysroot but that was reverted in #213185, and
remains a can of worms.

Signed-off-by: Peter Waller [email protected]

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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 Nov 15, 2024
@emilazy
Copy link
Member

emilazy commented Nov 15, 2024

Given that apparently the libc++ test case used to justify the LLVM patch previously now works without passing the flag at all (per discussion on Matrix), It would be good to verify if we actually need this. e.g., if we’re not passing it to GCC, is GCC currently leaky in the same way Clang would be without this flag? If so, then I don’t think it’s worth it to bother passing it at all, given the pitfalls.

cc @NixOS/llvm @NixOS/stdenv

@pwaller
Copy link
Contributor Author

pwaller commented Nov 15, 2024

if we’re not passing it to GCC, is GCC currently leaky in the same way Clang would be without this flag? If so, then I don’t think it’s worth it to bother passing it at all, given the pitfalls.

strace -efile -f gcc -c test.c |& grep usr gives empty output, so I figure it has the -nostdlibinc-ish behaviour needed in the current nixpkgs build.

@pwaller pwaller force-pushed the move-nostdlibinc-to-wrapper branch 2 times, most recently from b941415 to 39b8ec7 Compare November 15, 2024 13:57
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Yay.

Remove a patch to decrease the divergence from upstream.

Motivation is a continuation of trying to reduce the number of clang
builds floating around. Wrapper builds are cheap, compiler builds are
not.

It's expected that this patch can be removed at some point and replaced
with some other cc-wrapper logic. This will be a little bit easier with
the logic already in the cc-wrapper.

Ongoing discussion in NixOS#191152. It would be nice to drop this but we
think it's probably needed at least on systems where /usr/include may be
available to prevent the possibility of picking up unwanted includes.

One potential method for eliminating nostdlibinc in the future is by
providing a non-existent -sysroot but that was reverted in NixOS#213185, and
remains a can of worms.

Signed-off-by: Peter Waller <[email protected]>
@pwaller
Copy link
Contributor Author

pwaller commented Nov 15, 2024

Latest force removes extraneous blankline.

@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Nov 17, 2024
@emilazy
Copy link
Member

emilazy commented Nov 17, 2024

Successfully built pkgsLLVM.hello and firefox on aarch64-linux, I think this is good to go.

@emilazy emilazy merged commit 597a7ad into NixOS:staging Nov 17, 2024
34 of 35 checks passed
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: 501+ 10.rebuild-linux: 5001+ 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants