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: Unconditionally warn about skipped native flags #203675

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

zhaofengli
Copy link
Member

@zhaofengli zhaofengli commented Nov 29, 2022

Description of changes

This PR makes cc-wrapper warn about skipped native flags (e.g., -march=native) unconditionally, so the behavior is more visible for users using the wrapped compilers in a Nix shell environment where they would expect such flags to work.

Currently, cc-wrapper strips out such flags silently when NIX_ENFORCE_NO_NATIVE is enabled (the default in stdenv). A message is only emitted if NIX_DEBUG is set. This makes sense for reproduciblility, but the behavior can be surprising/undesirable to users using it interactively. The compiler shouldn't silently not do what is specified without informing the user.

The downside of adding such an unconditional warning is log spam during builds that specify any native flag. However, we should fix the affected packages to not use such flags in the first place. Currently, a full build of stdenv with this PR applied does not emit such warnings.

Alternative solutions
  1. Make cc-wrapper simply exit with a non-zero code when such flags are passed and NIX_ENFORCE_NO_NATIVE is enabled. This will probably break some packages, but is a cleaner solution.
  2. Make the default mkShell environment not have NIX_ENFORCE_NO_NATIVE.
Workarounds
  1. NIX_CFLAGS_COMPILE=-march=native produces the expected result since the flags are appended after filtering, but the effect pollutes the entire shell/build environment.
Things done
  • Built on platform(s)
    • x86_64-linux (built stdenv on master)
    • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@zhaofengli zhaofengli mentioned this pull request Nov 29, 2022
13 tasks
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023
@qweered
Copy link

qweered commented Oct 23, 2024

@Ericson2314

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 23, 2024
Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

Seems like a good idea

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 10, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2025
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.

I think a hard failure on NIX_ENFORCE_NO_NATIVE might be a good idea, but this seems like a reasonable way to help gauge what the fallout of that might be.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 5, 2025
@emilazy emilazy merged commit c648235 into NixOS:staging Jan 5, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants