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: Remove unused extraFlagsArray feature #69370

Merged
merged 1 commit into from
Oct 8, 2019
Merged

makeWrapper: Remove unused extraFlagsArray feature #69370

merged 1 commit into from
Oct 8, 2019
+5 −12

Conversation

chkno
Copy link
Member

@chkno chkno commented Sep 24, 2019 •
edited
Loading

Motivation for this change

There is a bug in this feature: It allows extra arguments to leak in
from the environment. For example:

$ export extraFlagsArray=date
$ man ls

Note that you get the man page for date rather than for ls. This happens
because man happens to use a wrapper (to add groff to its PATH).

An attempt to fix this was made in 5ae1857 in PR #19328 for
issue #2537, but 1. That change didn't actually fix the problem because
it addressed makeWrapper's environment during the build process, not the
constructed wrapper script's environment after installation, and 2. That
change was apparently accidentally lost when merged with 7ff6eec.

Rather than trying to fix the bug again, we remove the extraFlagsArray
feature, since it has never been used in the public repo in the ten
years it has been available.

wrapAclocal continues to use its own, separate flavor of extraFlagsArray
in a more limited context. The analogous bug there was fixed in
4d7d10d in 2011.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-review wip"
    • Tested compilation of all pkgs that depend on this change that are installed on my machine -- about 1000 packages
  • Tested execution of all binary files (usually in ./result/bin/)
    • Ran my personal computer with this change for a week
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@chkno chkno requested a review from Ericson2314 as a code owner September 24, 2019 18:14
@veprbl
Copy link
Member

veprbl commented Sep 24, 2019

The feature in question was introduced in e01be47

@veprbl veprbl requested a review from edolstra September 24, 2019 19:16
@chkno
Copy link
Member Author

chkno commented Sep 24, 2019

Fixed commit description: "seven years" -> "ten years".

@globin
Copy link
Member

globin commented Sep 25, 2019

Could you base this on the staging branch, please.

@chkno
makeWrapper: Remove unused extraFlagsArray feature
a45b3ad 
There is a bug in this feature: It allows extra arguments to leak in
from the environment. For example:

  $ export extraFlagsArray=date
  $ man ls

Note that you get the man page for date rather than for ls. This happens
because 'man' happens to use a wrapper (to add groff to its PATH).

An attempt to fix this was made in 5ae1857 in PR NixOS#19328 for
issue NixOS#2537, but 1. That change didn't actually fix the problem because
it addressed makeWrapper's environment during the build process, not the
constructed wrapper script's environment after installation, and 2. That
change was apparently accidentally lost when merged with 7ff6eec.

Rather than trying to fix the bug again, we remove the extraFlagsArray
feature, since it has never been used in the public repo in the ten
years it has been available.

wrapAclocal continues to use its own, separate flavor of extraFlagsArray
in a more limited context. The analogous bug there was fixed in
4d7d10d in 2011.
@chkno chkno changed the base branch from master to staging September 25, 2019 16:23
@chkno
Copy link
Member Author

chkno commented Sep 25, 2019

Switched to staging.

@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 6.topic: lua 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: pantheon The Pantheon desktop environment 6.topic: printing 6.topic: python 6.topic: qt/kde labels Sep 25, 2019
@ofborg ofborg bot added 8.has: module (update) This PR changes an existing module in `nixos/` 2.status: merge conflict This PR has merge conflicts with the target branch labels Sep 25, 2019
@globin
Copy link
Member

globin commented Sep 25, 2019

@GrahamcOfBorg eval

@veprbl
Copy link
Member

veprbl commented Sep 25, 2019

Eval issue should be resolved now
@GrahamcOfBorg eval

@ofborg ofborg bot removed 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 6.topic: lua 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: pantheon The Pantheon desktop environment 6.topic: printing 6.topic: python 6.topic: qt/kde 6.topic: ruby 6.topic: stdenv Standard environment 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 2.status: merge conflict This PR has merge conflicts with the target branch labels Sep 25, 2019
@globin globin merged commit 3564643 into NixOS:staging Oct 8, 2019
@arcnmx
Copy link
Member

arcnmx commented Oct 22, 2019

This just hit unstable yesterday and I use this pretty frequently, what is the recommended replacement? Something like --run 'set -- --something "$@"' maybe?

@chkno
Copy link
Member Author

chkno commented Oct 22, 2019

@arcnmx - Can you share an example of how you use it?

@arcnmx
Copy link
Member

arcnmx commented Oct 22, 2019

Hm mostly things like this and set does work fine. To be fair using run like that is probably an indicator that it's worth using an actual/custom wrapper anyway.

@chkno
Copy link
Member Author

chkno commented Oct 22, 2019

Cool, looks like you found a way to proceed. Sorry to cause you some churn.

Yes, it looks like --run 'set -- YOUR ARGS HERE "$@"' works as a general replacement for --run 'extraFlagsArray=(YOUR ARGS HERE)', and one that doesn't cause trouble for simpler uses of makeWrapper and wrapProgram the way extraFlagsArray did.

It looks like your wrappers end up coming out something like:

#! /nix/store/...-bash/bin/bash -e
export RUSTC_SYSROOT=${RUSTC_SYSROOT-'blah'}
[[ -z $RUSTC_SYSROOT || $* = *--sysroot* ]] || set -- --sysroot "$RUSTC_SYSROOT" "$@"
[[ -z $RUSTC_TARGET || $* = *--target* ]] || set -- --target "$RUSTC_TARGET" "$@"
[[ -z $RUSTC_FLAGS ]] || set -- $RUSTC_FLAGS "$@"
exec -a "$0" "/nix/store/.../bin/rustc" "$@"

This allows RUSTC_FLAGS from the wrapper's runtime environment to leak into the invocation of rustc, which in this case is both exactly what you want and exactly what it says on the tin -- it is not a surprise that RUSTC_FLAGS get passed to rustc.

Thanks for sharing your impact & fix!

@chkno chkno deleted the no-extra-flags branch November 4, 2019 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314

@edolstra edolstra

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chkno @veprbl @globin @arcnmx