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

gcc: tighten condition for inhibit_libc=true #241206

Merged
merged 1 commit into from Oct 5, 2023
Merged

gcc: tighten condition for inhibit_libc=true #241206

merged 1 commit into from Oct 5, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jul 3, 2023

Description of changes

The situation described in the comment preceding export inhibit_libc=true does not match the conditional which follows it. Specifically, the comment says that this line is meant for "clang builds gcc" situations, yet it is enabled even when !stdenv.cc.isClang.

This commit tightens the conditional to make it match the situation described in the comment.

Needed for:

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@trofi
Copy link
Contributor

trofi commented Jul 3, 2023

Why the change is needed? Does it fix anything?

If it does then I suggest update the the description to say so. And maybe comment should hint why it's undesirable for other cases.

gcc -> clang is only one example where config does not change. Another one would be pkgsCross.gnu64:

nix-repl> pkgsCross.gnu64.stdenv.hostPlatform == stdenv.hostPlatform
false

pkgsCross.gnu64 requires building a new libc without reliance on the host libc.

The situation described in the comment preceding `export
inhibit_libc=true` does not match the conditional which follows it.
Specifically, the comment says that this line is meant for "clang
builds gcc" situations, yet it is enabled even when
`!stdenv.cc.isClang`.

This commit tightens the conditional to make it match the situation
described in the comment.
@ghost
Copy link
Author

ghost commented Jul 12, 2023

Why the change is needed?

Because what the comment says and what the code does are not the same. One of them must change. If you prefer to change the comment please submit a PR that explains why we inhibit_libc in such a huge class of situations.

If it does then I suggest update the the description to say so.

I think it's the other way around -- right now we have this inhibit_libc=true with no description of why it's there (except if stdenv.cc.isClang).

Does it fix anything?

Yes, right now there's no way to not turn on inhibit_libc for the first-stage cross compiler, like #241208 does.

inhibit_libc doesn't just prevent gcc from using the buildPlatform headers -- it prevents it from using any headers at all. So if we already have the targetPlatform headers, this inhibit_libc is blocking us from being able to use them.

@trofi
Copy link
Contributor

trofi commented Jul 12, 2023

I'll remove myself from reviewers then.

@trofi trofi removed their request for review July 12, 2023 21:22
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 20, 2023
@ghost
Copy link
Author

ghost commented Jul 20, 2023

Rebased.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 20, 2023
@Artturin
Copy link
Member

Artturin commented Oct 4, 2023

Rebuilds caused by
nixpkgs_issue___screenshot_2023-10-04_22-53-28_123426745

The conditional was added in ea8e124 #182666

No diff in pkgsLLVM.hello

pkgsCross.aarch64-multiplatform.hello no longer has inhibit_libc=true

rnix-lsp
pkgsCross.aarch64-multiplatform.hello
pkgsCross.aarch64-multiplatform.pkgsLLVM.stdenv.cc

work

@Artturin Artturin merged commit c748544 into NixOS:master Oct 5, 2023
@yu-re-ka
Copy link
Contributor

yu-re-ka commented Oct 7, 2023

This broke pkgsMusl.nix:

       error: attribute 'useLLVM' missing

       at /nix/store/m2vqij78lfy9rvrmws264jpr7q78q98w-source/pkgs/development/compilers/gcc/common/pre-configure.nix:125:45:

          124|                       targetPlatform.config == hostPlatform.config &&
          125|                       (stdenv.cc.isClang || stdenv.targetPlatform.useLLVM)) ''
             |                                             ^
          126|   export inhibit_libc=true


$ nix eval -f . pkgsMusl.pkgsStatic.stdenv.targetPlatform.useLLVM
error: attribute 'useLLVM' in selection path 'pkgsMusl.pkgsStatic.stdenv.targetPlatform.useLLVM' not found

$ 

@Artturin
Copy link
Member

Artturin commented Oct 7, 2023

#259592

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Oct 9, 2023

This also broke pkgsMusl.pkgsStatic.buildPackages.gccWithoutTargetLibc.cc, which is also required for pkgsMusl.nix: https://termbin.com/wf5i

@alyssais
Copy link
Member

#260181

@ghost ghost deleted the tighten-inhibit_libc-condition branch January 23, 2024 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants