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.

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

makeWrapper: Remove unused extraFlagsArray feature #69370

Merged
merged 1 commit into from Oct 8, 2019

Conversation

@chkno
Copy link
Contributor

chkno commented Sep 24, 2019

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.
@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Sep 24, 2019

The feature in question was introduced in e01be47

@veprbl veprbl requested a review from edolstra Sep 24, 2019
@chkno chkno force-pushed the chkno:no-extra-flags branch from 03fa87c to a630c09 Sep 24, 2019
@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Sep 24, 2019

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

@globin

This comment has been minimized.

Copy link
Member

globin commented Sep 25, 2019

Could you base this on the staging branch, please.

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.
@chkno chkno force-pushed the chkno:no-extra-flags branch from a630c09 to a45b3ad Sep 25, 2019
@chkno chkno changed the base branch from master to staging Sep 25, 2019
@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Sep 25, 2019

Switched to staging.

@globin

This comment has been minimized.

Copy link
Member

globin commented Sep 25, 2019

@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Sep 25, 2019

Eval issue should be resolved now
@GrahamcOfBorg eval

@globin globin merged commit 3564643 into NixOS:staging Oct 8, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@arcnmx

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

chkno commented Oct 22, 2019

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

@arcnmx

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor 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 chkno:no-extra-flags branch Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.