Skip to content

stdenv: use named ref#130591

Merged
happysalada merged 1 commit intoNixOS:stagingfrom
happysalada:stdenv_use_named_ref
Oct 5, 2021
Merged

stdenv: use named ref#130591
happysalada merged 1 commit intoNixOS:stagingfrom
happysalada:stdenv_use_named_ref

Conversation

@happysalada
Copy link
Copy Markdown
Contributor

@happysalada happysalada commented Jul 19, 2021 •
edited
Loading

Motivation for this change

Instead of using an intermediary variable, use a named reference.
the named reference feature was introduced in bash 4.3 and is exactly the use case.
IMO it is an improvement, however I want to check if others feel that way toward using a named reference (there are other places where this can be used).

I had a PR previously to enable some stdenv "improvements" that actually broke the build (#127736)
The idea is to try to break that PR logically into commits so things can be easily accepted or rejected.
Some of the changes will be related to enabling another shell, some of the changes will be things that I think should be improvements.

I will try to mark each PR as "Can be controversial" vs "I think this is OK". The current PR IMO is "I think this is OK"

I will cc people that I think would be interested in this PR and just add in the comments the other PRs that are related.

@aszlig @matthewbauer
If you know of somebody else that could be interested, I would be happy to know.

Please note I'm only using master as a faster way to get some evaluations. My plan is to first break down the previous big PR into separate smaller ones. Then separately build on my machine the ones that don't fail the CI. Then ask someone from the hydra team to test them. (as the previous one broke quite some stuff and caused pain).

I also have some questions if anybody knows the answer.

  • Why are we not using bash 5 ? Is it because it's just a matter of making the PR to upgrade, or is there another reason?
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs 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 Relase 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 Jul 19, 2021
@happysalada happysalada force-pushed the stdenv_use_named_ref branch from 710fd9d to 6b70b79 Compare July 19, 2021 02:37
@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 19, 2021

PR that can be controversial
#130592
coming from commit 1c0dfca

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 19, 2021

PR that should be uncontroversial
#130593
coming from 3b32d74

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 19, 2021

Perhaps needs more checking
#130604

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 19, 2021

Potentially controversial PR #130608

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 19, 2021 •
edited
Loading

I think that sums up pretty much the last round of PRs. There are some more changes coming, but there shouldn't be that many more PRs coming.

Last but not least, I'm happy to revert any of those PR if you have strong feelings against any of them.

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 19, 2021

Controversial PR #130621

@Artturin
Copy link
Copy Markdown
Member

Artturin commented Jul 19, 2021 •
edited
Loading

These should target staging right?

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 19, 2021

@Artturin Thank you for your comment. I tried to explain in the PR. If I target staging, I'll probably never get ofborg to evaluate. As a hack I'm targeting master to get faster evaluation. Once people are happy, I will contact someone from the hydra team and will target staging. I'm still not sure this is the best way to proceed, I've seen other people doing it and it seems to work well.

@Artturin
Copy link
Copy Markdown
Member

Artturin commented Jul 19, 2021

@Artturin Thank you for your comment. I tried to explain in the PR. If I target staging, I'll probably never get ofborg to evaluate. As a hack I'm targeting master to get faster evaluation. Once people are happy, I will contact someone from the hydra team and will target staging. I'm still not sure this is the best way to proceed, I've seen other people doing it and it seems to work well.

okay 👍 i didn't see that comment

Ericson2314
Ericson2314 reviewed Jul 19, 2021
View reviewed changes
pkgs/stdenv/generic/setup.sh Outdated
Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 Jul 19, 2021

Choose a reason for hiding this comment

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

Should that be an and not or?

Copy link
Copy Markdown
Contributor Author

@happysalada happysalada Jul 19, 2021

Choose a reason for hiding this comment

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

I did mess it up! :-)

Copy link
Copy Markdown
Contributor Author

@happysalada happysalada Jul 19, 2021

Choose a reason for hiding this comment

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

I was really split between making it >1 || <-1 or keeping it like the original <= 1 && >= -1
(in the end I just mixed them up)

@happysalada happysalada force-pushed the stdenv_use_named_ref branch from 6b70b79 to 34bef33 Compare July 19, 2021 22:58
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 20, 2021
@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 23, 2021

This PR has to be tested thoroughly as I'm not sure what is happening with strings containing double @ signs in them.
#131160

I'm still "practicing" here and hope to get in the deep later.

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 23, 2021

semi-controversial ?
#131161
I'm trying to target staging now just to see if it changes the build priority by a large margin or not.

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 23, 2021

makes something a little more verbose
#131162

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 23, 2021

This is the last PR in this series for now
#131163

Hopefully I can merge some of those.
The next series will be targetted at trying to bring a new shell to nixpkgs.
My last attempt was enabling strict errexit by default and I learned that as a first step it should not be needed. So I will just try to run with the new shell with exactly the same options.
All the fixes proposed come from trying to run with strict errexit. I'm not convinced they are an improvement (some of those make things more verbose), but it seems that they would be needed at some point trying to enable all features for a new shell. So I thought I would put forward those improvements first to surface disagreements early.

@vcunat
Copy link
Copy Markdown
Member

vcunat commented Jul 26, 2021

Why are we not using bash 5 ? Is it because it's just a matter of making the PR to upgrade, or is there another reason?

I'm not aware. It seems quite probable that there's no reason against and noone's dared/bothered to look into switching the default so far. (introduced in PR #53628)

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 27, 2021

I'm trying an upgrade to bash 5 by default, let's see what the CI says
#131681

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Aug 2, 2021

@vcunat Just a friendly ping about testing a group of commit.
I've tried including the minimum set that would pass in my opinion.
I'm not sure what you want the testing process to be, I'm happy to adapt to what works best for you.
If you are busy, feel free to ignore this, I will ping again in 1 week.
Thank you for helping on this!

@happysalada happysalada mentioned this pull request Aug 3, 2021
Closed
11 tasks
@happysalada happysalada changed the base branch from master to staging August 30, 2021 01:29
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 30, 2021
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: python Python is a high-level, general-purpose programming language. 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 26, 2021
@github-actions github-actions bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: changelog This PR adds or changes release notes labels Sep 26, 2021
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 26, 2021
@happysalada happysalada mentioned this pull request Sep 26, 2021
Closed
12 tasks
@happysalada happysalada force-pushed the stdenv_use_named_ref branch from 3c3c123 to a3d67cb Compare October 1, 2021 12:45
@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Oct 5, 2021

hydra has passed, this PR is going to staging.

@happysalada happysalada merged commit 84e4715 into NixOS:staging Oct 5, 2021
@happysalada happysalada deleted the stdenv_use_named_ref branch October 5, 2021 15:42
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

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@happysalada @Artturin @vcunat @Ericson2314