buildPythonPackage, buildPythonApplication: support `__structuredAttrs = true` #347194

ShamrockLee · 2024-10-08T00:22:03Z

This PR refactors the setup hooks and wrap.sh that construct the two build helpers, making buildPythonPackage and buildPythonApplication support both __structuredAttrs = true and structuredAttrs = false.

The representation of the flags attributes as shell/environment variables for most Python building setup hooks are now the same as stdenv.mkDerivation and other build helpers -- they are space-separated environment variables when __structuredAttrs = false and Bash arrays when __structuredAttrs = true, and are concatenated to the command without Bash-evaluation. The following behaviour changes are introduced during the conversion:

The following flags are no longer Bash-expanded before concatenated to the command:
- disabledTests and disabledTestPaths for pytestCheckHook. (disabledTestPaths used to be expanded twice before concatenation.)
- setupPyBuildFlags and setupPyGlobalFlags for setuptoolsBuildHook.
pytestFlags and unittestFlags replaces pytestFlagsArray and unittestFlagsArray and become the new and conforming interface.
pytestFlagsArray and unittestFlagsArray are kept for compatibility purposes. They continue to be Bash-expanded before concatenated. Such compatibility layer would get removed in future releases.

Almost all the refactored scripts are linted with ShellCheck, except wrap.sh, which I don't know how to lint correctly.

Things done

Add a 👍 reaction to pull requests you find important.

ShamrockLee · 2024-10-09T13:53:54Z

Questions:

When passing makeWrapperArgs to buildPython* as an argument (attribute), it will be passed as an environment variable (containing a space-separated string) to the build script. However, people assume it would be a Bash array when changing it inside preFixup.

Should we adapt to it and transparently convert the values from the Nix derivation attribute to a Bash array as well, or should we fix the Nix expressions that treats them as arrays?
How should we fix the expressions mentioned above? Should we add __structuredAttrs = true to make makeWrapperArgs a Bash array, or should we use appendToVar to append elements to it? appendToVar inherently can't handle space-containing arguments. I'm unsure if adding __strucutredAttrs = true to a package expression would be considered backward-incompatible.

wolfgangwalther · 2024-10-09T15:24:10Z

Thanks for working on this! I intend to review this PR, but will not have much time the next two weeks.

ShamrockLee · 2024-10-09T20:59:39Z

@ofborg build python3Packages.six
@ofborg build python311Packages.tensorflow

wolfgangwalther

This is not a full review, but it should be enough for a first iteration.

Most importantly I have not touched the question on how to deal with the compatibility things, yet, aside from giving a few more generic thoughts on the matter.

Maybe we can split some of the uncontroversial changes off (everything independent of compatHelpers) and merge separately?

pkgs/development/interpreters/python/hooks/pip-build-hook.sh

pkgs/development/interpreters/python/hooks/pip-install-hook.sh

pkgs/development/interpreters/python/hooks/pypa-build-hook.sh

pkgs/development/interpreters/python/hooks/python-output-dist-hook.sh

pkgs/development/interpreters/python/hooks/python-relax-deps-hook.sh

pkgs/development/interpreters/python/hooks/setuptools-rust-hook.sh

pkgs/development/python-modules/meson-python/add-build-flags.sh

ShamrockLee · 2024-10-27T18:05:24Z

This confuses me. Why are we still using +=" ..." string concatenation for Phases at all? It seems like all hooks use xxxHooks+=(...) array syntax already, so maybe we should just do the same for Phases separately? Seems to be only preFixupPhases in ~8 files right now.

There used to be a lot of *Phase+=" somePhase" before #339117. #339117 also targets *Phases+=(somePhase), but some were not caught up as they are more challenging to discover using regular expression search.

It seems like all hooks use xxxHooks+=(...) array syntax already, so maybe we should just do the same for Phases separately?

When __structuredAttrs = false, The preFixupPhases = [ "foo" "bar" ] will pass to the build script as an environment variable preFixupPhases="foo bar". When someone does preFixupPhases+=("baz"), it will be transparently converted to preFixupPhases+=("foo bar" "baz"). Such value is semantically wrong but tolerated by the setup.sh implementation as it is accessed as "${preFixupPhases[*]}".

We should not rely on such implementation detail of setup.sh, but handle the *Phases correctly with prependToVar and appendToVar.

ShamrockLee · 2024-10-27T18:13:00Z

This is not a full review, but it should be enough for a first iteration.

Thank you for reviewing!

Most importantly I have not touched the question on how to deal with the compatibility things, yet, aside from giving a few more generic thoughts on the matter.

Maybe we can split some of the uncontroversial changes off (everything independent of compatHelpers) and merge separately?

I'll split the easier part into another PR after we reach a consensus about how to add elements to *Phases. Or should we discuss it in the new PR?

wolfgangwalther · 2024-10-27T18:27:10Z

There used to be a lot of *Phase+=" somePhase" before #339117. #339117 also targets *Phases+=(somePhase), but some were not caught up as they are more challenging to discover using regular expression search.

I wasn't aware of that PR. I see that doing the same for the last remaining *Phases here makes sense. But in that case, this should be a commit doing all of the remaining ones together? (grep -r 'Phases+=' pkgs still shows me ~8 more on your branch)

ShamrockLee · 2024-10-27T18:37:17Z

(grep -r 'Phases+=' pkgs still shows me ~8 more on your branch)

Oops! My bad.

I see that doing the same for the last remaining *Phases here makes sense. But in that case, this should be a commit doing all of the remaining ones together? (grep -r 'Phases+=' pkgs still shows me ~8 more on your branch)

I'll gather them into the same commit.

Update: Applied.

wolfgangwalther · 2024-12-22T16:32:10Z

The current pytestCheckHook implementation turns out to chek for the existence of each path inside disabledTestPaths and throw build-time errors if they doesn't exist, causing the globbed paths to fail.

should we drop the path existence check or add a new flag for the globs? If the latter, how should we name the new attribute?

Ah, interesting. How about we change the existence check to only throw an error if no glob is present?

So globbed paths go through unchecked, while unglobbed paths are checked.

ShamrockLee · 2024-12-22T17:12:42Z

How about we change the existence check to only throw an error if no glob is present?

So globbed paths go through unchecked, while unglobbed paths are checked.

Sounds great!

However, it's challenging to differentiate globbed paths from unglobbed ones. Formally speaking, we are trying to detect if a glob matches and only matchs a path identical to itself when an infinite set of paths is tried.

wolfgangwalther · 2024-12-22T17:16:21Z

However, it's challenging to differentiate globbed paths from unglobbed ones. Formally speaking, we are trying to detect if a glob matches and only matchs a path identical to itself when an infinite set of paths is tried.

Uhm, I thought we'd just check whether a literal * is present in the test path? If yes, then pytest will glob for it. If no, then we check existence.

ShamrockLee · 2024-12-22T17:23:32Z

Searching across the PyTest source code, I found that it uses the fnmatch module from the Python standard library to match the globs against a path PyTest collects in former steps.

If we would like to, we could use the glob module from the Python standard library to check with Python if each glob matches some paths in CWD.

ShamrockLee · 2024-12-22T17:27:46Z

Uhm, I thought we'd just check whether a literal * is present in the test path? If yes, then pytest will glob for it. If no, then we check existence.

This implementation would assume:

Users would only use * in the globs.
The paths to ignore must not contain *, ?, [ or ], or the user will be forced to escape it with [<char>] and fail the path existence check.

wolfgangwalther · 2024-12-22T17:44:14Z

If we would like to, we could use the glob module from the Python standard library to check with Python if each glob matches some paths in CWD.

That would be best, of course.

Add flag pytestFlags as the new, conforming interface replacing pytestFlagsArray. Stop Bash-expanding disabledTests and disabledTestPaths. Handle disabledTestPaths with `pytest --ignore-glob <path>` to keep globbing support. Check if each path glob matches at least one path using the `glob` module from the Python standard library. Also make buildPythonPackage and buildPythonApplication stop escaping the elements of disabledTests and disabledTestPaths.

Handle flags with appendToVar and concatTo. Stop Bash-expanding elements of setupPyGlobalFlags and setupPyBuildFlags.

…ithout expecting Bash expansion

…expanding expection

…stically Take unittestFlags as the new and conforming interface. Keep unittestFlagsArray as is.

…s support

ShamrockLee · 2024-12-22T20:04:23Z

@ofborg build python3Packages.conda
@ofborg build python3Packages.tensorflow

wolfgangwalther

I did the following:

went through the PR line-by-line, except for a few nits the changes LGTM.
found no remaining uses of pytestFlagsArray or unittestFlagsArray in doc/*.
found no other setupPyGlobalFlags or setupPyBuildFlags depending on bash eval or passing spaces.
build some random python packages to confirm nothing is obviously broken.

wolfgangwalther · 2025-01-06T20:20:39Z