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

makeWrapper: add --set-default #31767

Merged
merged 4 commits into from Nov 17, 2017
Merged

makeWrapper: add --set-default #31767

merged 4 commits into from Nov 17, 2017

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Nov 17, 2017 •
edited

Motivation for this change

#31497 (comment)

After #31497 starter quoting all values, there arouse the need to left some values evaluated.

--set-default var value expands to export var=${var-value}, where value is not evaluated and literally assigned to var unless it is already set.

--set-eval var value expands to export var=$(eval echo value), where value is evaluated by eval. --run 'export var=expr' should be used instead.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@orivej
makeWrapper: add --set-default and --set-eval
4199892 
After NixOS#31497 starter quoting all values, there arouse the need to left some
values evaluated.

`--set-default var value` expands to `export var=${var-value}`, where value is
not evaluated and literally assigned to var unless it is already set.

`--set-eval var value` expands to `export var=$(eval echo value)`, where value
is evaluated by `eval`.
@orivej orivej requested a review from abbradar November 17, 2017 10:20
@orivej orivej force-pushed the make-wrapper branch 2 times, most recently from 7d721f8 to 7eb4941 Compare November 17, 2017 11:30
@orivej orivej changed the title [WIP] makeWrapper: add --set-default and --set-eval makeWrapper: add --set-default and --set-eval Nov 17, 2017
@orivej orivej changed the title makeWrapper: add --set-default and --set-eval [WIP] makeWrapper: add --set-default and --set-eval Nov 17, 2017
@orivej
mesos: do not use makeWrapper with bash from bootstrap tools
80d1eaa 
It is too old and does not support required features. The error is:

/nix/store/...-hook/nix-support/setup-hook: line 78: export $varName=${value@Q}\${$varName:+${separator@Q}}\$$varName: bad substitution
@orivej
treewide: use --set-default and --set-eval where appropriate
74648f7
@orivej orivej changed the title [WIP] makeWrapper: add --set-default and --set-eval makeWrapper: add --set-default and --set-eval Nov 17, 2017
@orivej
Copy link
Contributor Author

orivej commented Nov 17, 2017

I consider this ready for staging: --set-default and --set-eval behave as expected, they did not require any changes since I've submitted the PR. The packages touched by the second and third commits are properly wrapped. If something is missing, it will not cause a mass rebuild.

abbradar
abbradar approved these changes Nov 17, 2017
View reviewed changes
Copy link
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

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

I don't see any issues, especially in make-wrapper.sh, but anyway you seem to be better versed in Bash quoting than me.

FWIW I used --run to achieve something similar recently.

@orivej
Copy link
Contributor Author

orivej commented Nov 17, 2017 •
edited

Good idea! --set-default is a clear win for clarity, but --set-eval is not good enough to be used instead of --run, I'll delete it.

@orivej
makeWrapper: delete --set-eval
fbdc74c 
because --run is good enough.
@orivej orivej changed the title makeWrapper: add --set-default and --set-eval makeWrapper: add --set-default Nov 17, 2017
@orivej orivej merged commit eb36534 into NixOS:staging Nov 17, 2017
@orivej orivej deleted the make-wrapper branch November 17, 2017 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abbradar abbradar abbradar approved these changes

Assignees
No one assigned
Labels
1.severity: mass-rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@orivej @abbradar @vcunat @GrahamcOfBorg