Skip to content

Commit

Permalink
python.pkgs.wrapPython: fix string makeWrapperArgs
Browse files Browse the repository at this point in the history 
Bash takes an assignment of a string to an array variable:

local -a user_args
user_args="(foo bar)"

to mean appending the string to the array, not parsing the string into
an array as is the case when on the same line as the declaration:

local -a user_args="(foo bar)"

b063340 extracted the declaration before
the newly branched code block, causing string makeWrapperArgs being added
to the array verbatim.

Since local is function scoped, it does not matter if we move it inside
each of the branches so we fix it this way.
  • Loading branch information
@jtojnar
jtojnar committed Dec 28, 2019
1 parent 4a2621d commit a6bb2ed
Showing 1 changed file with 2 additions and 3 deletions.
5 changes: 2 additions & 3 deletions pkgs/development/interpreters/python/wrap.sh
View file
Expand Up @@ -81,14 +81,13 @@ wrapPythonProgramsIn() {

# Add any additional arguments provided by makeWrapperArgs
# argument to buildPythonPackage.
local -a user_args
# We need to support both the case when makeWrapperArgs
# is an array and a IFS-separated string.
# TODO: remove the string branch when __structuredAttrs are used.
if [[ "$(declare -p makeWrapperArgs)" =~ ^'declare -a makeWrapperArgs=' ]]; then
user_args=("${makeWrapperArgs[@]}")
local -a user_args=("${makeWrapperArgs[@]}")
else
user_args="($makeWrapperArgs)"
local -a user_args="($makeWrapperArgs)"
fi

local -a wrapProgramArgs=("${wrap_args[@]}" "${user_args[@]}")
Expand Down

8 comments on commit a6bb2ed

@FRidh
Copy link
Member

@FRidh FRidh commented on a6bb2ed Dec 29, 2019

Choose a reason for hiding this comment

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

/nix/store/nriqys8a294bn95a3zxb8rj1i1hra2jf-hook/nix-support/setup-hook: line 110: declare: makeWrapperArgs: not found

@jtojnar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What package produces that?

@jtojnar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I forgot wrapPython can be used even outside mkPythonDerivation. But I would expect that to fail even before b063340 with set -u, as we used "($makeWrapperArgs)" before too.

@FRidh
Copy link
Member

@FRidh FRidh commented on a6bb2ed Dec 29, 2019

Choose a reason for hiding this comment

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

I've seen it with many packages, e.g.

$ nix-build -A python3.pkgs.tox

@jtojnar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot reproduce that. Getting a different error instead:

ERROR: Could not find a version that satisfies the requirement importlib-metadata<2,>=1.1.0; python_version < "3.8" (from tox==3.14.2) (from versions: none)
ERROR: No matching distribution found for importlib-metadata<2,>=1.1.0; python_version < "3.8" (from tox==3.14.2)

@FRidh
Copy link
Member

@FRidh FRidh commented on a6bb2ed Dec 29, 2019

Choose a reason for hiding this comment

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

This was using #76059

@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented on a6bb2ed Dec 29, 2019 •
edited

Choose a reason for hiding this comment

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

Oh, there is NixOS/nix#1461, so makeWrapperArgs in fact are not passed to wrapPython in most cases.

nix-repl> ({ name, makeWrapperArgs ? [] }@attrs: attrs) { name = "foo"; }
{ name = "foo"; }

But weirdly, on #76059, nix-build -A python3.pkgs.tox succeeds for me (even with the error being printed). Are we not setting set -u in Python setup hooks?

@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented on a6bb2ed Dec 29, 2019 •
edited

Choose a reason for hiding this comment

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

Should be fixed with 21574b2.

The fact that -o nounset is not enabled in the setup hook is weird. I would expect #72347 to enable it there as well.

And even when I try to enable it in the script

--- a/pkgs/development/interpreters/python/wrap.sh
+++ b/pkgs/development/interpreters/python/wrap.sh
@@ -1,3 +1,5 @@
+set -o nounset
+
 # Wrapper around wrapPythonProgramsIn, below. The $pythonPath
 # variable is passed in from the buildPythonPackage function.
 wrapPythonPrograms() {
@@ -79,6 +81,7 @@ wrapPythonProgramsIn() {
                         wrap_args+=(--set PYTHONNOUSERSITE "true")
                     fi
 
+                    shopt -o nounset || true
                     # Add any additional arguments provided by makeWrapperArgs
                     # argument to buildPythonPackage.
                     # We need to support both the case when makeWrapperArgs

by the time wrapPythonPrograms is run in postFixup, it is disabled again.

Please sign in to comment.