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: revert "do not install sys-include headers for cross-compilers." #245550

Merged
1 commit merged into from
Jul 28, 2023

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Jul 26, 2023

The change reverts commit 7df4387

A few reasons to revert the commit:

  1. The change was not enough to restore -idirafter override semantic to match unwrapped compiler.
  2. The change broke override semantics for cross-compilers
  3. The change made override semantics different between cross-compiler and native compiler

All of three have some overlap between, but I think it's important to call all of them out.

The main fallout is the uboot builds, reported by cynerd.

Used the following test to check the override recovery:

$ nix shell github:NixOS/nixpkgs/release-22.05#pkgsCross.aarch64-multiplatform.stdenv.cc
$$ cat stdio.h
# empty
$$ printf "#include <stdio.h>" | aarch64-unknown-linux-gnu-gcc -E - -o - -idirafter . >/dev/null; echo $?
0

It failed before the change and succeded after.

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.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.

The change reverts commit 7df4387

A few reasons to revert the commit:
1. The change was not enough to restore `-idirafter` override semantic
   to match unwrapped compiler.
2. The change broke override semantics for cross-compilers
3. The change made override semantics different between cross-compiler
   and native compiler

All of three have some overlap between, but I think it's important
to call all of them out.

The main fallout is the uboot builds, reported by cynerd.

Used the following test to check the override recovery:

    $ nix shell github:NixOS/nixpkgs/release-22.05#pkgsCross.aarch64-multiplatform.stdenv.cc
    $$ cat stdio.h
    # empty
    $$ printf "#include <stdio.h>" | aarch64-unknown-linux-gnu-gcc -E - -o - -idirafter . >/dev/null; echo $?
    0

It failed before the change and succeded after.
@trofi trofi requested a review from matthewbauer as a code owner July 26, 2023 15:44
@trofi trofi requested a review from a user July 26, 2023 15:44
@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Jul 26, 2023
@trofi
Copy link
Contributor Author

trofi commented Jul 26, 2023

Validated the fix helped for a smaller test:

$ nix shell -f. pkgsCross.aarch64-multiplatform.stdenv.cc
$$ touch stdio.h
$$ printf "#include <stdio.h>" | aarch64-unknown-linux-gnu-gcc -E - -o - -idirafter . >/dev/null; echo $?
0

@trofi
Copy link
Contributor Author

trofi commented Jul 26, 2023

And also tested on @Cynerd 's config as:

$ nix build .#legacyPackages.x86_64-linux.pkgsCross.aarch64-multiplatform.ubootTurrisMox --override-input nixpkgs ~/patched-staging

The change recovered the build.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This is a very good idea. Especially because the conditional can't be known at eval-time, so this if-then-fi can't be lifted into Nix code.

I will merge this as soon as my rebuilds complete.

Building on:

  • x86_64-linux
  • powerpc64le-linux
  • aarch64-linux (cross from x86_64-linux)
  • mips64el-linux mixed n32/64 (cross from x86_64-linux)

Also built:

  • pkgs.tests.cross.sanity (almost finished)

@ghost ghost merged commit 8470989 into NixOS:staging Jul 28, 2023
@trofi trofi deleted the gcc-restore-sys-include branch July 28, 2023 06:06
@trofi
Copy link
Contributor Author

trofi commented Jul 30, 2023

Caused bootstrap failure on aarch64-linux: #246147 .Looks like it exposed use of old libgcc against new libstdc++ headers.

@ghost
Copy link

ghost commented Jul 31, 2023

Caused bootstrap failure on aarch64-linux: #246147 .Looks like it exposed use of old libgcc against new libstdc++ headers.

This turned out to be due to parallel stripping, rather than libgcc:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant