Skip to content

treewide: move NIX_CFLAGS_COMPILE to the env attrset #217206

Merged
lovesegfault merged 6 commits intoNixOS:masterfrom
Artturin:stdenvimprovements1
Feb 23, 2023
Merged

treewide: move NIX_CFLAGS_COMPILE to the env attrset #217206
lovesegfault merged 6 commits intoNixOS:masterfrom
Artturin:stdenvimprovements1

Conversation

@Artturin
Copy link
Copy Markdown
Member

@Artturin Artturin commented Feb 19, 2023 •
edited
Loading

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 Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: mate The MATE Desktop Environment 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: printing Drivers, CUPS & Co. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: stdenv Standard environment 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 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 mentioned this pull request Feb 19, 2023
Open
@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 This PR removes packages or removes other cruft 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
@Artturin Artturin force-pushed the stdenvimprovements1 branch from 5c2c130 to 3251013 Compare February 22, 2023 19:23
@Artturin Artturin changed the base branch from staging to master February 22, 2023 19:23
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 22, 2023
@Artturin Artturin marked this pull request as ready for review February 22, 2023 22:40
@Artturin Artturin removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 22, 2023
@marsam marsam mentioned this pull request Feb 23, 2023
Merged
12 tasks
@wegank
Copy link
Copy Markdown
Member

wegank commented Feb 23, 2023 •
edited
Loading

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
Copy Markdown
Member Author

Artturin commented Feb 23, 2023 •
edited
Loading

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
Copy Markdown
Member

7c6f434c commented Feb 23, 2023 via email

@wegank
Copy link
Copy Markdown
Member

wegank commented Feb 23, 2023

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

@Atemu
Copy link
Copy Markdown
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
Copy Markdown
Member Author

Artturin 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?

see commit messages

@Artturin
Copy link
Copy Markdown
Member Author

Artturin commented Feb 23, 2023 •
edited
Loading

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
Merged
12 tasks
@Artturin
Copy link
Copy Markdown
Member Author

Artturin commented Feb 24, 2023 •
edited
Loading

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

@Artturin Artturin mentioned this pull request Feb 24, 2023
Closed
12 tasks
@wineee wineee mentioned this pull request Mar 1, 2023
Merged
12 tasks
@markuskowa markuskowa mentioned this pull request Mar 14, 2023
Merged
@Shawn8901 Shawn8901 mentioned this pull request Apr 11, 2023
Closed
@uninsane uninsane mentioned this pull request May 29, 2023
Merged
12 tasks
@OPNA2608 OPNA2608 mentioned this pull request Jul 11, 2023
Merged
12 tasks
@wegank wegank mentioned this pull request Nov 18, 2023
Merged
13 tasks
@ghost ghost mentioned this pull request Dec 4, 2023
Closed
13 tasks
@magneticflux- magneticflux- mentioned this pull request Jan 13, 2024
Merged
13 tasks
@ConnorBaker ConnorBaker mentioned this pull request Sep 13, 2024
Open
4 tasks
@yshui yshui mentioned this pull request Nov 8, 2024
Merged
13 tasks
@wolfgangwalther wolfgangwalther mentioned this pull request May 4, 2025
Closed
13 tasks
@caffineehacker caffineehacker mentioned this pull request Sep 3, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wegank wegank wegank approved these changes

@jtojnar jtojnar Awaiting requested review from jtojnar

@mmahut mmahut Awaiting requested review from mmahut

@RaghavSood RaghavSood Awaiting requested review from RaghavSood

@aanderse aanderse Awaiting requested review from aanderse

@etu etu Awaiting requested review from etu

@globin globin Awaiting requested review from globin

@Ma27 Ma27 Awaiting requested review from Ma27

@talyz talyz Awaiting requested review from talyz

@adisbladis adisbladis Awaiting requested review from adisbladis

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

@marsam marsam Awaiting requested review from marsam

@ttuegel ttuegel Awaiting requested review from ttuegel

@ajs124 ajs124 Awaiting requested review from ajs124

@lukegb lukegb Awaiting requested review from lukegb

@mweinelt mweinelt Awaiting requested review from mweinelt

Assignees

No one assigned

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 Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: mate The MATE Desktop Environment 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: printing Drivers, CUPS & Co. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: stdenv Standard environment 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 6.topic: xfce The Xfce Desktop Environment 8.has: clean-up This PR removes packages or removes other cruft 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Artturin @wegank @7c6f434c @Atemu @lovesegfault