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

cc-wrapper: conditionalize the nonexistent sysroot hack #213738

Closed
wants to merge 2 commits into from
Closed

cc-wrapper: conditionalize the nonexistent sysroot hack #213738

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 31, 2023

Description of changes
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.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

trofi and others added 2 commits January 30, 2023 23:47
… from libc

I would like to add an extra `gcc` build step during linux bootstrap
(#208412). This makes it early
bootstrap compiler linked and targeted against `bootstrapTools` `glibc`
including it's headers.

Without this change `gcc`'s spec files always prefer `bootstrapTools` `glibc`
for header search path (passed in as --with-native-system-header-dir=). We'can't
override it with:

- `-I` option as it gets stacked before gcc-specific headers, we need to keep
  glibc headers after gcc as gcc cleans namespace up for C standard by using
  #include_next and by undefining system macros.
- `-idirafter` option as it gets appended after existing `glibc`-includes

This `--sysroot=/nix/store/does/not/exist` hack allows us to remove existing
`glibc` headers and add new ones with `-idirafter`.

We use `cc-cflags-before` instead of `libc-cflags` to allow user to define
their own `--sysroot=` (like `firefox` does).

To keep it working prerequisite cross-symlink in gcc.libs is required:
#209153
Copy link
Contributor

@trofi trofi left a comment

Choose a reason for hiding this comment

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

I don't think --sysroot=/nix/store/does/not/exist can be used as is without addressing library search paths and include search paths.

+ optionalString (libc != null) (''
touch "$out/nix-support/libc-cflags"
touch "$out/nix-support/libc-ldflags"
echo "-B${libc_lib}${libc.libdir or "/lib/"}" >> $out/nix-support/libc-crt1-cflags
'' + optionalString enableNonExistentSysroot ''
echo "--sysroot=/nix/store/does/not/exist" >> $out/nix-support/cc-cflags-before
Copy link
Contributor

Choose a reason for hiding this comment

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

As stated in #213185 blanket --sysroot= changes library search path in transitive libraries. As a result we get linkage failures in dmd, d-seams, llvmPackages_rocm.compiler-rt. Conditional or not it's breaking real packages. The intent in the comment was only attempting to change header search path and not library search path.

For just headers there is an -isysroot flag. But it has it's own caveats as blanket --sysroot= also exposed failures in ipxe and wimboot. Those show that existing -idirafter sequence is incorrectly overridden in nixpkgs. It has to be fixed by chaning a priority.

Copy link
Author

Choose a reason for hiding this comment

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

The conditional is only enabled for xgcc, which is never used to compile any of those packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be to explore ways to add the flag locally to the place where it's intended to be used instead of changing the driver. I still think it's too heavy a hammer even for gcc.

@ghost ghost requested review from trofi and removed request for Ericson2314 January 31, 2023 08:11
+ optionalString (libc != null) (''
touch "$out/nix-support/libc-cflags"
touch "$out/nix-support/libc-ldflags"
echo "-B${libc_lib}${libc.libdir or "/lib/"}" >> $out/nix-support/libc-crt1-cflags
'' + optionalString enableNonExistentSysroot ''
echo "--sysroot=/nix/store/does/not/exist" >> $out/nix-support/cc-cflags-before
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be to explore ways to add the flag locally to the place where it's intended to be used instead of changing the driver. I still think it's too heavy a hammer even for gcc.

@wegank wegank mentioned this pull request Feb 1, 2023
4 tasks
@ghost
Copy link
Author

ghost commented Feb 1, 2023

I think I have a solution that will make everybody happy: no patches to gcc and does not use nonexistent-sysroot at all.

I'm running some test builds overnight and will clean it up in the morning, but the key insight is in:

@ghost ghost closed this Feb 1, 2023
@ghost ghost deleted the pr/sysroot-hack/conditionalize branch January 23, 2024 06:50
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
This pull request was closed.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants