Skip to content

stdenv: use command that doesn't exit non-zero#130592

Closed
happysalada wants to merge 1 commit intoNixOS:stagingfrom
happysalada:stdenv_errexit_fixes
Closed

stdenv: use command that doesn't exit non-zero#130592
happysalada wants to merge 1 commit intoNixOS:stagingfrom
happysalada:stdenv_errexit_fixes

Conversation

@happysalada
Copy link
Copy Markdown
Contributor

@happysalada happysalada commented Jul 19, 2021

Motivation for this change

This is a PR in the series of stdenv shell improvements.

This current PR replaces a command that exits with a non-zero code on failure with one that does not.
The code is in my opinion a test semantic rather than an "exit on failure" one.
This change serves in enabling strict err exit at some point.
In the previous commit @zimbatm pointed that the semantics are a little different.
If the $hookName is a directory the semantics change. In my experience this cannot be possible, however this is something that will need to be verified with the hydra tests.

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 mentioned this pull request Jul 19, 2021
Merged
11 tasks
@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 19, 2021
veprbl
veprbl reviewed Jul 23, 2021
View reviewed changes
@happysalada happysalada force-pushed the stdenv_errexit_fixes branch from 06e2093 to 090b162 Compare July 23, 2021 08:03
@happysalada happysalada changed the base branch from master to staging July 23, 2021 08:03
@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 23, 2021

I'm not sure what the failure is on the tests. Maybe it's just a transient thing.

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 27, 2021

The failures appears to be

2021-07-23T12:31:38.5744656Z Errors while running CTest
2021-07-23T12:31:38.5770383Z make: *** [Makefile:126: test] Error 8
2021-07-23T12:31:38.6593610Z builder for '/nix/store/s5dm9vhx44sz965ccwb4q8xan8s5mzn2-aws-c-common-0.6.8.drv' failed with exit code 2
2021-07-23T12:31:38.6630851Z cannot build derivation '/nix/store/59b8wi04ai4apbk638qblxhm9b9xyl6p-aws-sdk-cpp-1.8.121.drv': 1 dependencies couldn't be built
2021-07-23T12:31:41.2115195Z building '/nix/store/w064yafzhw0apwpflf1lwwbr9al0nad3-brotli-1.0.9.drv'...
2021-07-23T12:31:41.2213371Z building '/nix/store/0qj5iqdxyi64xwgiwgf3ypz4r0z3yz29-nlohmann_json-3.9.1.drv'...
2021-07-23T12:31:41.2305852Z cannot build derivation '/nix/store/9xiy2hdxhqiiibb0y21wxb6qg1p6acrs-nix-2.3.14.drv': 1 dependencies couldn't be built
2021-07-23T12:31:41.2312217Z cannot build derivation '/nix/store/n5z2lgm8zv0q97vz7b1z6gmhrvnp2583-nixpkgs-release-checks.drv': 1 dependencies couldn't be built
2021-07-23T12:31:41.2413350Z error: build of '/nix/store/n5z2lgm8zv0q97vz7b1z6gmhrvnp2583-nixpkgs-release-checks.drv' failed
2021-07-23T12:31:41.2455710Z ##[error]Process completed with exit code 100.
2021-07-23T12:31:41.2572093Z Post job cleanup.
2021-07-23T12:31:41.5772807Z [command]/usr/bin/git version
2021-07-23T12:31:41.5865508Z git version 2.32.0
2021-07-23T12:31:41.5919795Z [command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
2021-07-23T12:31:41.5968411Z [command]/usr/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :
2021-07-23T12:31:41.6467729Z [command]/usr/bin/git config --local --name-only --get-regexp http\.https\:\/\/github\.com\/\.extraheader
2021-07-23T12:31:41.6498723Z http.https://github.com/.extraheader
2021-07-23T12:31:41.6515394Z [command]/usr/bin/git config --local --unset-all http.https://github.com/.extraheader
2021-07-23T12:31:41.6556814Z [command]/usr/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :
2021-07-23T12:31:41.7077396Z Cleaning up orphan processes

zimbatm
zimbatm reviewed Jul 27, 2021
View reviewed changes
pkgs/stdenv/generic/setup.sh
local hookName="$2"
if declare -F "$hookName" > /dev/null; then
"$hookName"
elif type -p "$hookName" > /dev/null; then
Copy link
Copy Markdown
Member

@zimbatm zimbatm Jul 27, 2021

Choose a reason for hiding this comment

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

This is not semantically equivalent.

  • type -p looks up the PATH for a program called "$hookName". If it exists, it outputs its location and exits with 0. Otherwise it exits with 1.
  • test -e checks if a file located at "$hookName" exists. If it does, it exits with 0. Otherwise it exits with 1.

Copy link
Copy Markdown
Member

@zimbatm zimbatm Jul 27, 2021

Choose a reason for hiding this comment

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

There is a case where both would yield the same result, if $hookName points to a relative and executable path.

Copy link
Copy Markdown
Contributor Author

@happysalada happysalada Jul 28, 2021

Choose a reason for hiding this comment

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

Thank you for the feeback!
I think you're right here and that change is a mistake.
I'm going to do one more test, but it seems weird to me that the original code is generating an error. From what I understood, in bash, if a command is in an if statement then the errexit should be ignored.
Perhaps the strict_all flag of osh is removing that behavior.
I'll test again and will ask andrew if that is the case.

@veprbl veprbl marked this pull request as draft October 3, 2021 04:29
@stale
Copy link
Copy Markdown

stale bot commented Apr 16, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 16, 2022
@happysalada happysalada deleted the stdenv_errexit_fixes branch April 28, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zimbatm zimbatm zimbatm left review comments

@veprbl veprbl veprbl left review comments

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees

No one assigned

Labels

2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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

No open projects
Staging
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@happysalada @zimbatm @veprbl