Skip to content

stdenv: introduce withCFlags#135533

Merged
tomberek merged 1 commit intoNixOS:masterfrom
MatthewCroughan:stdenv-adapters-useTheseFlags
Jan 30, 2022
Merged

stdenv: introduce withCFlags#135533
tomberek merged 1 commit intoNixOS:masterfrom
MatthewCroughan:stdenv-adapters-useTheseFlags

Conversation

@MatthewCroughan
Copy link
Contributor

@MatthewCroughan MatthewCroughan commented Aug 24, 2021

Motivation for this change

This PR adds an easy method of appending compiler flags to your stdenv via a list.

It adds a new function called withCFlags which lets anyone create a new stdenv with a list of compiler flags, or anything one might want to add to the CLI of each gcc invocation.

In my own system, I wanted to have everything use -O3 and -funroll-loops so I did that like this.

{
  nixpkgs.overlays = [
    (self: super: {
      stdenv = super.withCFlags [ "-funroll-loops" "-O3" ] super.stdenv;
    })
  ];
}
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 via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Aug 24, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Aug 24, 2021
@MatthewCroughan MatthewCroughan changed the title stdenv: introduce useTheseFlags stdenv: introduce withCFlags Aug 24, 2021
@sternenseemann
Copy link
Member

Don't feel like stdenvAdapters is the right place for this since this doesn't change a stdenv, but overrides stdenv for a package (which is then returned).

@MatthewCroughan
Copy link
Contributor Author

MatthewCroughan commented Aug 24, 2021

@sternenseemann
Then should this function that is already in adapters.nix not be here? There are a few others that I believe are the same as what I'm doing. This is why I thought adapters.nix is exactly the right place for this function. If not, where are you suggesting it go instead?

Edit: Are you replying to Gytis' #135533 (comment) comment here? It seems like what you're saying fits that more.
https://github.com/NixOS/nixpkgs/blob/691ab87f0121b87009cf7e52e206a5a5652e5af3/pkgs/stdenv/adapters.nix#L218-L224

@sternenseemann
Copy link
Member

Ah sorry, you are right, I misread your diff. This indeed belongs in there, although I'd suggest the function should not take an attribute set, but instead two arguments: withCFlags = flags: stdenv: to be more consistent with the rest of the adapters.

The functions suggested in #135533 (comment) OTOH belong somewhere else since they interact with packages, not stdenvs.

@MatthewCroughan
Copy link
Contributor Author

MatthewCroughan commented Aug 25, 2021

Ah sorry, you are right, I misread your diff. This indeed belongs in there, although I'd suggest the function should not take an attribute set, but instead two arguments: withCFlags = flags: stdenv: to be more consistent with the rest of the adapters.

The functions suggested in #135533 (comment) OTOH belong somewhere else since they interact with packages, not stdenvs.

@sternenseemann I made an attrset on the advice of @cleverca22 who suggested the errors are more appropriate when you don't supply one of the arguments. If I were to do it like you suggested, then it would not be clear in the error message which argument was missing. This way, it will fail and report in the log that "compilerFlags" or "stdenv" is missing, by name.

@sternenseemann
Copy link
Member

@sternenseemann I made an attrset on the advice of @cleverca22 who suggested the errors are more appropriate when you don't supply one of the arguments. If I were to do it like you suggested, then it would not be clear in the error message which argument was missing. This way, it will fail and report in the log that "compilerFlags" or "stdenv" is missing, by name.

But then it's much more cumbersome to use currying to make additional adapters out of the adapter:

let
  optimize2Adapter = withCFlags [ "-O2" ];
  sanitizeAdapter = withCFlags [ "-fsanitize=address" ];
in

versus

let
  optimize2Adapter = stdenv:  withCFlags { compilerFlags = [ "-O2" ]; inherit stdenv; };
  sanitizeAdapter = stdenv: withCFlags { compilerFlags = [ "-fsanitize=address" ]; inherit stdenv; };
in

Since all adapters are made like I suggested, the consistency should make this quite obvious, whereas you need to remember the compilerFlags argument in the second case even.

@MatthewCroughan
Copy link
Contributor Author

MatthewCroughan commented Jan 11, 2022

@sternenseemann After learning more about Nix, I think what you're exactly right. Do you have any more suggestions? If not, I'll squash and someone can merge it.

@MatthewCroughan MatthewCroughan force-pushed the stdenv-adapters-useTheseFlags branch 2 times, most recently from 2cf74f4 to 9c8fff6 Compare January 11, 2022 21:37
Adds an easy method of appending compiler flags to your stdenv via a
list.

Co-authored-by: tomberek <tomberek@users.noreply.github.com>
Co-authored-by: Gytis Ivaskevicius <gytis02.21@gmail.com>
Co-authored-by: sternenseemann <sternenseemann@systemli.org>
@MatthewCroughan MatthewCroughan force-pushed the stdenv-adapters-useTheseFlags branch from 9c8fff6 to ef33586 Compare January 12, 2022 12:47
@SuperSandro2000 SuperSandro2000 removed their request for review January 25, 2022 17:38
@SuperSandro2000
Copy link
Member

Not my area of expertise I must admit. Didn't see anything obvious but might have missed something.

@MatthewCroughan
Copy link
Contributor Author

@SuperSandro2000 No problem, thanks for looking.

@tomberek tomberek merged commit b0c0e0d into NixOS:master Jan 30, 2022
@MatthewCroughan MatthewCroughan deleted the stdenv-adapters-useTheseFlags branch August 3, 2022 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants