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.

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

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

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.

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?

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)

(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.

FTR: I built conda and tensorflow successfully on x86_64-linux, when applying this set of patches on master. Let's fix the nits / docs / merge-conflict and merge this, I'd say.

Thank you both for reviewing! I'll address them next week (after the final exam of my uni).

Good luck with exams!

Addressed the review suggestions and rebased!

Now we can start migrating pytestFlagsArray to pytestFlags. I'll add a TODO in the tracking issue