python3Packages.*Hook: support __structuredAttrs = true (the easier ones)

[ShamrockLee]

This are the easier, less controversial part of the effort to make buildPythonPackage and buildPythonApplication support both __structuredAttrs = true and structuredAttrs = false.

Each modified Bash script is linted with ShellCheck.

It also fixes some leftovers of #339117 among Python-related hooks.

Split from #347194

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[wolfgangwalther]

Just read through, so far. Now we need packages that actually make use of those flags and test them with structuredAttrs and without:

• pipBuildFlags... is not used at all, so no useful package to test.
• pipInstallFlags.. used 3 times, but only with a single flag. Not useful to test.
• pypaBuildFlags could be tested with pillow-jpls. This should fail with structuredAttrs on master/staging, if I'm not mistaken, but pass with structuredAttrs in this PR.
• pythonImportsCheck should be tested carefully with one of the many packages that has multiple items set. This would likely fail silently with structuredAttrs on master/staging, only testing the first list item, but would test all of them with this branch.
• pythonNamespaces also needs to be checked manually, to confirm the cleanup was in fact broken when multiple namespaces were given with structuredAttrs, but is fixed now.
• pythonRelaxDeps and pythonRemoveDeps should equally be tested manually - the build with structuredAttrs would probably not outright fail, yet, but the metadata file be subtly broken. Should be fixed with this branch.

[ShamrockLee]

Now we need packages that actually make use of those flags and test them with structuredAttrs and without:

How about adding some packages to the passthru.tests of the hooks and checking others manually?

• pythonImportsCheck should be tested carefully with one of the many packages with multiple items set. This would likely fail silently with structuredAttrs on master/staging, testing only the first list item, but would test all of them with this branch.

In my experience, pythonImportsCheckHook is what most Python packages on master would stumble upon switching __structuredAttrs = true. Fixing pythonImportsChceckHook alone would make most packages pass the build.

[ShamrockLee]

pypaBuildFlags could be tested with pillow-jpls. This should fail with structuredAttrs on master/staging, if I'm not mistaken, but pass with structuredAttrs in this PR.

pillow-jpls has a strange pypaBuildFlags specification containing single quotes, which seems to expect Bash expansion before execution. However, pypaBuildHook does not evaluate the constructed command before execution like pytestCheckHook does. I wonder if pillow-jpls would ever build the way the package author expects.

Cc: @bcdarwin (python3Packages.pillow-jpls maintainer)

[ShamrockLee]

Update: python3Packages.pillow-jpls works for both __structuredAttrs = true and structuredAttrs = false, but the build logs of both cases contain the following message:

*** Configuring CMake...
CMake Warning:
  Ignoring extra path from command line:

   "'--preset=sysdeps'"

This line is also in the build log of python3Packages.pillow-jpls on the master branch.

Submitting #352042 to fix this.

[wolfgangwalther]

python3Packages.pillow-jpls works for both __structuredAttrs = true and structuredAttrs = false, but the build logs of both cases contain the following message:

Right. And with structuredAttrs = true it fails on current master. So the change to pypaBuildFlags works as intended.

How about adding some packages to the passthru.tests of the hooks and checking others manually?

Hm, I think the most valuable part to test for me is, to confirm that a package fails on master currently with structuredAttrs - but then passes with structuredAttrs on this branch. This needs to be tested manually anyway, to confirm the package broke before. So I don't see much point in adding those passthru.tests right now, if the idea is to eventually switch to "build everthing with structuredAttrs by default". Because then, this will be tested already, but we will surely forget to remove the passthru tests.

[wolfgangwalther]

In my experience, pythonImportsCheckHook is what most Python packages on master would stumble upon switching __structuredAttrs = true. Fixing pythonImportsChceckHook alone would make most packages pass the build.

I don't understand that, yet. Here's what I understand about the challenge with structuredAttrs:

• A nix list of pythonImportsCheckHook would be passed as a bash array.
• The bash code doesn't expect that and accesses $pythonImportsCheckHook only, which will only ever return the first array element.
• In other cases (flags etc.) this could cause a build failure, but for import checks it would just cause fewer import checks to actually run.

So pythonImportsCheckHook is the one that I expect to not cause any failures at all, but instead silently do bad.

[wolfgangwalther]

So pythonImportsCheckHook is the one that I expect to not cause any failures at all, but instead silently do bad.

I tested it and now I understand what you mean: Because the python code accesses this variable as an environment variable it works without structuredAttrs. But with structuredAttrs this is not an exported environment variable anymore, but just a bash variable - thus python can't read it, and the imports check fails indeed.

[ShamrockLee]

I tested it and now I understand what you mean: Because the python code accesses this variable as an environment variable it works without structuredAttrs. But with structuredAttrs this is not an exported environment variable anymore, but just a bash variable - thus python can't read it, and the imports check fails indeed.

Exactly. The previous implementation of pythonImportsCheckHook relies on pythonImportsCheck being passed to the Python code as an environment variable.

[wolfgangwalther]

I tested all commits, they do what they claim to do - fix those hooks with structuredAttrs turned on.

While the sum of changes is good, the split across commits is still not 100% clean. For some hooks shellcheck and structuredAttrs changes claim to be split between two commits, but the structuredAttrs commit contains part of the shellcheck changes.

The first commit should better be treewide: and fix the remaining cases of that the *Phases+= pattern as well.

The namespace hook should be a single shellcheck-only commit.

Other than that - it works.

[wolfgangwalther]

This needs a rebase for the base branch to evaluate.

LGTM otherwise!

Thanks for pushing structuredAttrs :)

[emilazy]

This looks good and indeed easy to me! Some questions to make sure I understand everything fully.

[emilazy]

Not blocking, but: would be nice to do a trick like that /dev/null thing to get declarations of all these essential variables.

[wolfgangwalther]

The /dev/null / source stuff was there before, see my reasoning in #351734 (comment) for removing it for now.

[emilazy]

Is this needed, or just general clean‐up of the example?

[ShamrockLee]

This change makes it closer to how an actual project would be structured. The resulting package after this change could be Python-imported like import my_project.

Such change is not strictly necessary to build the test anyway.

[philiptaron]

Should we merge this in the next couple days?

[emilazy]

I was waiting for @ShamrockLee to rebase per responses to review feedback before merging.

[ShamrockLee]

Rebased.

[emilazy]

LGTM, thanks!