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.

Sign up for GitHub

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

Jump to bottom

stdenv: enable __structuredAttrs #72074

Closed
wants to merge 337 commits into from
Closed

stdenv: enable __structuredAttrs #72074

wants to merge 337 commits into from

Conversation

globin
Copy link
Member

@globin globin commented Oct 27, 2019 •
edited
Loading

__structuredAttrs is a nice feature, we want it enabled in stdenv!

hydra jobset: https://hydra.nixos.org/jobset/nixpkgs/structured-attrs

related: NixOS/rfcs#13

Further information: https://nixos.mayflower.consulting/blog/2020/01/20/structured-attrs/

cc @Ericson2314 @matthewbauer

@FRidh FRidh added the 2.status: work-in-progress This PR isn't done label Oct 27, 2019
@FRidh FRidh added this to the 20.03 milestone Oct 27, 2019
@ofborg ofborg bot added 6.topic: ruby 6.topic: stdenv Standard environment 8.has: documentation This PR adds or changes documentation labels Oct 27, 2019
@globin globin mentioned this pull request Oct 27, 2019
Merged
10 tasks
@ofborg ofborg bot added 6.topic: erlang 6.topic: golang 6.topic: haskell 6.topic: lua 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 6.topic: xfce The Xfce Desktop Environment 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: rust 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: qt/kde labels Oct 27, 2019
@ofborg ofborg bot added the 6.topic: pantheon The Pantheon desktop environment label Oct 30, 2019
@jtojnar
Copy link
Member

jtojnar commented Oct 30, 2019

Why not make the NIX_*_FLAGS be lists as well?

@globin
Copy link
Member Author

globin commented Oct 30, 2019 •
edited
Loading

They need to be exported to be picked up by bintools-/cc-wrapper

@Ericson2314
Copy link
Member

@globin we could require that they be lists, but then convert to strings (and quote!) ourselves. If that is not more churn, I think i might prefer that.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 30, 2019
@Ericson2314 Ericson2314 mentioned this pull request Oct 31, 2019
Merged
2 tasks
@ofborg ofborg bot added 8.has: changelog and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Nov 2, 2019
@Ericson2314
Copy link
Member

I added set -u as I got that ready/pretty close, and ensured at least stdenv built after the merge. Also fixed an eval failure.

@ofborg ofborg bot added 2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: emacs Text editor labels Nov 3, 2019
Ericson2314
Ericson2314 reviewed Nov 4, 2019
View reviewed changes
pkgs/stdenv/generic/setup.sh
@@ -827,7 +827,7 @@ unpackFile() {
unpackPhase() {
runHook preUnpack

if [ -z ${srcs+"${srcs[@]}"} ]; then
if [ -z ${srcs-"${srcs[@]}"} ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

${srcs-"${srcs[@]}"} expands to the first element of the srcs array; surely that’s not the intention.

Suggested change
if [ -z ${srcs-"${srcs[@]}"} ]; then
if [ ${#srcs[@]} = 0 ]; then

@vcunat vcunat added this to the 21.11 milestone Apr 30, 2021
@Ma27 Ma27 mentioned this pull request May 4, 2021
Merged
4 tasks
@github-actions github-actions bot removed 6.topic: stdenv Standard environment 6.topic: printing 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: bsd Running or building packages on BSD 6.topic: systemd 6.topic: vim 6.topic: steam Steam game store/launcher (store.steampowered.com) labels May 24, 2021
@Mindavi
Copy link
Contributor

Mindavi commented Oct 13, 2021

Is there still time to push this in? If not, is there any way this can be split up to make it more bearable to get this in? Otherwise this will be pushed forward indefinitely, I'm afraid.

@Artturin
Copy link
Member

Artturin commented Oct 14, 2021 •
edited
Loading

i think a way to help could be:
fork nixpkgs and checkout the structured-attrs branch
run git rebase -i origin/master and solve the merge conflicts (300 or so)
this should also clean up the commit list and order it

@Artturin
Copy link
Member

what if we created a structuredAttrsStdenv and migrated expressions gradually?

@Mindavi
Copy link
Contributor

Mindavi commented Nov 27, 2021

At least making it so this change can be split up seems like a good idea to me. I don't mind hacking at this, but doing everything at once is very hard to do, it'll also quickly have conflicts.

@kamadorueda kamadorueda mentioned this pull request Nov 27, 2021
Closed
13 tasks
@Artturin
Copy link
Member

Artturin commented May 4, 2022

@Mindavi I found #85042
Which has structuredAttrs as a opt in

@Artturin
Copy link
Member

#175649

@Artturin Artturin mentioned this pull request May 31, 2022
Merged
13 tasks
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/cmakeflags-and-spaces-in-option-values/20170/2

@jtojnar jtojnar mentioned this pull request Oct 14, 2022
Merged
13 tasks
@Artturin
Copy link
Member

#175649 has been merged

@Artturin Artturin closed this Dec 11, 2022
@Mindavi Mindavi deleted the structured-attrs branch December 11, 2022 11:17
@Mindavi Mindavi restored the structured-attrs branch December 11, 2022 11:18
@Artturin Artturin mentioned this pull request Feb 24, 2023
Closed
12 tasks
@wegank wegank deleted the structured-attrs branch May 27, 2023 12:51
@ConnorBaker ConnorBaker mentioned this pull request Sep 13, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Ericson2314 left review comments

@andersk andersk andersk left review comments

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: work-in-progress This PR isn't done 6.topic: agda "A dependently typed programming language / interactive theorem prover" 6.topic: cinnamon Desktop environment 6.topic: emacs Text editor 6.topic: erlang 6.topic: fetch 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang 6.topic: haskell 6.topic: kernel The Linux kernel 6.topic: lua 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: ocaml 6.topic: pantheon The Pantheon desktop environment 6.topic: policy discussion 6.topic: python 6.topic: qt/kde 6.topic: ruby 6.topic: rust 6.topic: xfce The Xfce Desktop Environment 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/`
Projects
None yet
Milestone
21.11
Development

Successfully merging this pull request may close these issues.