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

treewide: move NIX_CFLAGS_COMPILE to the env attrset #217206

Merged
merged 6 commits into from
Feb 23, 2023

Conversation

Artturin
Copy link
Member

@Artturin Artturin commented Feb 19, 2023

Description of changes

with structuredAttrs lists will be bash arrays which cannot be exported
which will be a issue with some patches and some wrappers like cc-wrapper

Previous unmerged structuredAttrs and env PRs have done the same thing #76732

other variables that will have to be transferred (in separate PRs)

NIX_CFLAGS_LINK
NIX_LDFLAGS
NIX_PATH
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
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: emacs Text editor 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: kernel The Linux kernel 6.topic: lua 6.topic: mate The MATE Desktop Environment 6.topic: ocaml 6.topic: printing 6.topic: python 6.topic: qt/kde 6.topic: ruby 6.topic: stdenv Standard environment 6.topic: systemd 6.topic: xfce The Xfce Desktop Environment labels Feb 19, 2023
@Artturin Artturin force-pushed the stdenvimprovements1 branch from da3f2b3 to 9fd7fdd Compare February 19, 2023 19:30
@Artturin Artturin force-pushed the stdenvimprovements1 branch 3 times, most recently from 2ea0c08 to 1997d46 Compare February 20, 2023 15:18
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Feb 20, 2023
@Artturin Artturin force-pushed the stdenvimprovements1 branch 7 times, most recently from d648301 to c8cc1ba Compare February 20, 2023 19:09
@ofborg ofborg bot added 8.has: clean-up 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 Feb 20, 2023
@Artturin Artturin force-pushed the stdenvimprovements1 branch from c8cc1ba to 3dbd481 Compare February 21, 2023 18:12
@wegank
Copy link
Member

wegank commented Feb 23, 2023

Looks like passing env.NIX_CFLAGS_COMPILE through overrideDerivation is causing errors:

$ nix-build -A rPackages.data_table
error: cannot coerce a set to a string

       at /.../nixpkgs/pkgs/development/r-modules/default.nix:978:7:

          977|     data_table = old.data_table.overrideDerivation (attrs: {
          978|       env.NIX_CFLAGS_COMPILE = attrs.NIX_CFLAGS_COMPILE + " -fopenmp";
             |       ^
          979|       patchPhase = "patchShebangs configure";
(use '--show-trace' to show detailed location information)

Also for emscriptenPackages.zlib.

@Artturin
Copy link
Member Author

Artturin commented Feb 23, 2023

They should be changed to use overrideAttrs https://nixos.org/manual/nixpkgs/stable/#sec-pkg-overrideDerivation

Do not use this function in Nixpkgs as it evaluates a Derivation before modifying it, which breaks package abstraction and removes error-checking of function arguments

i can do it, should be a simple replacement

@7c6f434c
Copy link
Member

7c6f434c commented Feb 23, 2023 via email

@wegank
Copy link
Member

wegank commented Feb 23, 2023

Aha! attrs.NIX_CFLAGS_COMPILE should be attrs.env.NIX_CFLAGS_COMPILE instead.

@Atemu
Copy link
Member

Atemu commented Feb 23, 2023

$ nix-build -A glm
error: The ‘env’ attribute set can only contain derivation, string, boolean or integer attributes. The ‘NIX_CFLAGS_COMPILE’ attribute is of type list.
(use '--show-trace' to show detailed location information)

I take it this makes it impossible to use a list of strings for cflags?

@Artturin
Copy link
Member Author

$ nix-build -A glm
error: The ‘env’ attribute set can only contain derivation, string, boolean or integer attributes. The ‘NIX_CFLAGS_COMPILE’ attribute is of type list.
(use '--show-trace' to show detailed location information)

I take it this makes it impossible to use a list of strings for cflags?

see commit messages

@Artturin
Copy link
Member Author

Artturin commented Feb 23, 2023

rPackages emscriptenPackages fixes #217870

appears to be a problem in only those sets

checked with git diff --name-only @ @~6 | xargs rg overrideDerivation on this branch

@Atemu Atemu mentioned this pull request Feb 23, 2023
12 tasks
Atemu added a commit to Atemu/nixpkgs that referenced this pull request Feb 23, 2023
@Artturin
Copy link
Member Author

Artturin commented Feb 24, 2023

with #217962 commits other than treewide: move NIX_CFLAGS_COMPILE to the env attrset could be reverted (other than some manual fixups

markuskowa added a commit to Nix-QChem/NixOS-QChem that referenced this pull request Mar 14, 2023
Change was introduced in NixOS/nixpkgs#217206
for structuredAttrs
@uninsane uninsane mentioned this pull request May 29, 2023
12 tasks
@wegank wegank mentioned this pull request Nov 18, 2023
13 tasks
magneticflux- added a commit to magneticflux-/nixpkgs that referenced this pull request Jan 15, 2024
Closes NixOS#273746
See NixOS#217206

Co-authored-by: Weijia Wang <9713184+wegank@users.noreply.github.com>
artyrian pushed a commit to artyrian/nixpkgs that referenced this pull request Jun 27, 2024
Closes NixOS#273746
See NixOS#217206

Co-authored-by: Weijia Wang <9713184+wegank@users.noreply.github.com>
@yshui yshui mentioned this pull request Nov 8, 2024
13 tasks
uninsane pushed a commit that referenced this pull request Nov 14, 2024
See #217206

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
hatch01 pushed a commit to hatch01/nixpkgs that referenced this pull request Dec 7, 2024
See NixOS#217206

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: emacs Text editor 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: kernel The Linux kernel 6.topic: lua 6.topic: mate The MATE Desktop Environment 6.topic: ocaml 6.topic: printing 6.topic: python 6.topic: qt/kde 6.topic: ruby 6.topic: stdenv Standard environment 6.topic: systemd 6.topic: xfce The Xfce Desktop Environment 8.has: clean-up 8.has: documentation This PR adds or changes documentation 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