treewide: handle `*Phases` variables `__structuredAttrs`-agnostically by ShamrockLee · Pull Request #339117 · NixOS/nixpkgs

ShamrockLee · 2024-09-02T22:03:12Z

Description of changes

Always specify the preDistPhases attribute as a list instead of a string.

Append elements to the preDistPhases Bash variable using appendToVar instead of string or Bash array concatenation.

Handle element insertion before a specific element using string substitution as before, but handle both structured and unstructured attributes.

State explicitly in the Nixpkgs Manual, "*Phases attributes are lists" and "elements of *Phases variables contain no spaces."

Things done

Add a 👍 reaction to pull requests you find important.

Always specify the prePhases attribute as a list instead of a string. Append elements to the prePhases Bash variable using appendToVar instead of string or Bash array concatenation.

Always specify the preConfigurePhases attribute as a list instead of a string. Append elements to the preConfigurePhases Bash variable using appendToVar instead of string or Bash array concatenation.

Always specify the preInstallPhases attribute as a list instead of a string. Append elements to the preInstallPhases Bash variable using appendToVar instead of string or Bash array concatenation.

Always specify the preDistPhases attribute as a list instead of a string. Append elements to the preDistPhases Bash variable using appendToVar instead of string or Bash array concatenation. Handle element insertion before a specific element using string substitution as before, but handle both structured and unstructured attributes.

Always specify the postPhases attribute as a list instead of a string. Append elements to the postPhases Bash variable using appendToVar instead of string or Bash array concatenation.

Require the elements of *Phases not to contain spaces. Require the *Phases attribute to be specified as Nix Language lists.

corngood

+1 for dotnet stuff. I promise to keep an eye out for this in the future. :)

ShamrockLee · 2024-09-06T20:17:07Z

Test buildVimPlugin:
@ofborg build spacevim

Test autoreconfHook:
@ofborg build voms
@ofborg build dasher

Test gimpPlugins:
@ofborg build gimpPlugins

I'm unsure how to test the pkgs/build-support/coq/default.nix changes. Ping @CohenCyril

I'm unsure how to test the makeDarwinBundle changes. Ping @hexagonal-sun

I'm unsure how to test releaseTools. Ping @edolstra

Test hareHook:
@ofborg build bonsai
@ofborg build haredo

I'm unsure how to test the setup hook of gnustep-make. Ping @ashalkhakov @matthewbauer @2xsaiko

Test xfce4.setupHook
@ofborg build xfce.xfce4-i3-workspaces-plugin

Test dotnet-sdk-setup-hook.sh:
@ofborg build nbxplorer

Test writeRequiredOctavePackagesHooks:
@ofborg build octavePackages.audio

Test pytestCheckHook:
@ofborg build python3Packages.six

Test pytest-forked's setup hook:
@ofborg build python3Packages.httplib2

Test pytest-xdist's setup hook:
@ofborg build python3Packages.quodlibet

Test pytest_7's setup hook:
@ofborg build python3Packages.graphviz

Test the setup hook of pytest for Python 2 (PyPy is still supporting Python2, and will continue to support it):
@ofborg build pypy2Packages.six

Test glib's setup hook.
@ofborg build image-roll

Test liquidfun:
@ofborg build liquidfun

I'm unsure how to test the changes in qtbase-setup-hook.sh qmake-hook.sh for Qt 5 and 6. Ping @ttuegel

I'm unsure how to test the changes in pkgs/servers/home-assistant/build-custom-component/manifest-requirements-check-hook.sh. Ping @graham33

emilazy · 2024-09-06T20:20:00Z

Test the setup hook of pytest for Python 2 (PyPy is still supporting Python2, and will continue to support it): @ofborg build pypy2Packages.six

(FWIW although PyPy2 will of course continue to be packaged, I’m quite sure python2-modules will go away as soon as the new GIMP comes out and I figure out how to get resholve onto the C++ OSH. resholve would be a more relevant test here.)

philiptaron

One two things in here look scary at all to me.

I did a "quick" staging build on those two and everything came up roses. 🌹🌹🦌🌹🌹

Result of nixpkgs-review pr 339117 -p python3Packages.pytest-forked -p python3Packages.pytest-xdist run on x86_64-linux 1

4 packages built:

python3Packages.pytest-forked
python3Packages.pytest-forked.dist (python3Packages.pytest-forked.dist.dist)
python3Packages.pytest-xdist
python3Packages.pytest-xdist.dist (python3Packages.pytest-xdist.dist.dist)

pkgs/development/python-modules/pytest-forked/setup-hook.sh

pkgs/development/python-modules/pytest-xdist/setup-hook.sh

philiptaron · 2024-09-06T23:55:40Z

I intend to merge this within the next day.

ShamrockLee · 2024-09-07T00:19:24Z

One two things in here look scary at all to me.

I did a "quick" staging build on those two and everything came up roses. 🌹🌹🦌🌹🌹

Result of nixpkgs-review pr 339117 -p python3Packages.pytest-forked -p python3Packages.pytest-xdist run on x86_64-linux 1

4 packages built:

python3Packages.pytest-forked

python3Packages.pytest-forked.dist (python3Packages.pytest-forked.dist.dist)

python3Packages.pytest-xdist

python3Packages.pytest-xdist.dist (python3Packages.pytest-xdist.dist.dist)

It would be great if they could be handled more gracefully (e.g. without string substitutions or if-blocks). Still, I wonder if it worths it to craft a generic global Bash helper function (like appendToVar) for such a specific use case.

philiptaron · 2024-09-07T05:22:38Z

It would be great if they could be handled more gracefully (e.g. without string substitutions or if-blocks). Still, I wonder if it worth it to craft a generic global Bash helper function (like appendToVar) for such a specific use case.

Yeah, I see no need to do so at the present juncture; it's likely that these two derivations could get golfed into something neater, in fact I'm certain of it, but no need to make the perfect be the enemy of this quite good PR.

Artturin · 2024-10-31T23:05:39Z

This might have caused #351010 "${mingw-patch}/*.patch" to break

ShamrockLee · 2024-10-31T23:33:40Z

This might have caused #351010 "${mingw-patch}/*.patch" to break

This PR changed the semantics of the __structuredAttrs = false packages only when changing from fooPhases+=(myPhase) to appendToVar fooPhases myPhase.

The breakage of patches = [ "${mingw-patch}/*patch" ] seems like an issue of the globbing handling of patches, but the diff of this PR hasn't touched the code processing patches.

Does patches = [ "${mingw-patch}/*patch" ] work before the merging commit of this PR but break afterward?

greg-hellings · 2024-11-04T01:35:43Z

This might have caused #351010 "${mingw-patch}/*.patch" to break

This PR changed the semantics of the __structuredAttrs = false packages only when changing from fooPhases+=(myPhase) to appendToVar fooPhases myPhase.

The breakage of patches = [ "${mingw-patch}/*patch" ] seems like an issue of the globbing handling of patches, but the diff of this PR hasn't touched the code processing patches.

Does patches = [ "${mingw-patch}/*patch" ] work before the merging commit of this PR but break afterward?

Doing a bisect on the problematic branches, it appears that the breakage appeared here which doesn't seem to be part of this branch.

emilazy · 2024-11-04T01:44:19Z

cc @wolfgangwalther

ShamrockLee added 6 commits September 3, 2024 05:33

treewide: handle prePhases __structuredAttrs-agnostically

21c0129

Always specify the prePhases attribute as a list instead of a string. Append elements to the prePhases Bash variable using appendToVar instead of string or Bash array concatenation.

treewide: handle preConfigurePhases __structuredAttrs-agnostically

5d42a8b

Always specify the preConfigurePhases attribute as a list instead of a string. Append elements to the preConfigurePhases Bash variable using appendToVar instead of string or Bash array concatenation.

treewide: handle preInstallPhases __structuredAttrs-agnostically

385d523

Always specify the preInstallPhases attribute as a list instead of a string. Append elements to the preInstallPhases Bash variable using appendToVar instead of string or Bash array concatenation.

treewide: handle postPhases __structuredAttrs-agnostically

758056d

Always specify the postPhases attribute as a list instead of a string. Append elements to the postPhases Bash variable using appendToVar instead of string or Bash array concatenation.

doc/stdenv: document the format of *Phases

5ec4f67

Require the elements of *Phases not to contain spaces. Require the *Phases attribute to be specified as Nix Language lists.

ShamrockLee marked this pull request as ready for review September 6, 2024 18:02

ShamrockLee requested review from Ericson2314, K900, NickCao, SuperSandro2000, corngood, figsoda, mweinelt, natsukium, philiptaron and ttuegel as code owners September 6, 2024 18:02

corngood approved these changes Sep 6, 2024

View reviewed changes

wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 6, 2024

philiptaron approved these changes Sep 6, 2024

View reviewed changes

pkgs/development/python-modules/pytest-forked/setup-hook.sh Show resolved Hide resolved

pkgs/development/python-modules/pytest-xdist/setup-hook.sh Show resolved Hide resolved

wegank added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Sep 7, 2024

philiptaron merged commit 4160ccc into NixOS:staging Sep 7, 2024

ConnorBaker mentioned this pull request Sep 13, 2024

[Tracking Issue] __structuredAttrs TODOs and FIXMEs #205690

Open

4 tasks

This was referenced Oct 27, 2024

buildPythonPackage, buildPythonApplication: support __structuredAttrs = true #347194

Merged

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

Merged

ShamrockLee deleted the structured-attrs-phases branch October 31, 2024 17:37

ShamrockLee mentioned this pull request Oct 31, 2024

treewide: handle *Phases variables __structuredAttrs-agnostically (round 2) #352709

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

treewide: handle `*Phases` variables `__structuredAttrs`-agnostically#339117

treewide: handle `*Phases` variables `__structuredAttrs`-agnostically#339117
philiptaron merged 6 commits intoNixOS:stagingfrom
ShamrockLee:structured-attrs-phases

ShamrockLee commented Sep 2, 2024

Uh oh!

corngood left a comment

Uh oh!

ShamrockLee commented Sep 6, 2024

Uh oh!

emilazy commented Sep 6, 2024

Uh oh!

philiptaron left a comment

Uh oh!

Uh oh!

Uh oh!

philiptaron commented Sep 6, 2024

Uh oh!

ShamrockLee commented Sep 7, 2024

Uh oh!

philiptaron commented Sep 7, 2024

Uh oh!

Artturin commented Oct 31, 2024

Uh oh!

ShamrockLee commented Oct 31, 2024 •

edited

Loading

Uh oh!

greg-hellings commented Nov 4, 2024

Uh oh!

emilazy commented Nov 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

ShamrockLee commented Sep 2, 2024

Description of changes

Things done

Uh oh!

corngood left a comment

Choose a reason for hiding this comment

Uh oh!

ShamrockLee commented Sep 6, 2024

Uh oh!

emilazy commented Sep 6, 2024

Uh oh!

philiptaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

philiptaron commented Sep 6, 2024

Uh oh!

ShamrockLee commented Sep 7, 2024

Uh oh!

philiptaron commented Sep 7, 2024

Uh oh!

Artturin commented Oct 31, 2024

Uh oh!

ShamrockLee commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg-hellings commented Nov 4, 2024

Uh oh!

emilazy commented Nov 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ShamrockLee commented Oct 31, 2024 •

edited

Loading