build-support/php: support `lib.extendMkDerivation` by drupol · Pull Request #385830 · NixOS/nixpkgs

drupol · 2025-02-28T16:14:53Z

The PHP builders were supporting the finalAttrs pattern from the beginning (thanks @jtojnar for this!). This PR update the builders using lib.extendMkDerivation.

Inspired from #382550

Context: #234651

Things done

Add a 👍 reaction to pull requests you find important.

…poserProject2` Inspired from NixOS#382550 Context: NixOS#234651

…erVendor` Inspired from NixOS#382550 Context: NixOS#234651

drupol · 2025-02-28T17:20:45Z

@ofborg build phpunit phpPackages.psalm

JohnRTitor

Passes my eyeball test

ShamrockLee · 2025-03-02T20:24:56Z

Going through the diff, this PR seems to change the way vendorHash gets overridden. The build helpers previously supports <php package>.overrideAttrs { vendorHash = <new hash>; }, but now the overriding is only possible via overriding composerVendor.

jtojnar · 2025-03-02T21:18:12Z

@JohnRTitor For non-leaf packages, and especially far reaching changes like this, there should be a longer review period. Probably at least a week, since many contributors only have time for review e.g. during weekends.

ShamrockLee · 2025-03-02T21:37:06Z

Going through the diff, this PR seems to change the way vendorHash gets overridden. The build helpers previously supports <php package>.overrideAttrs { vendorHash = <new hash>; }, but now the overriding is only possible via overriding composerVendor.

If this interface change is intended, there should be a release note entry under the "Backward incompatibility" sub-section. If not, it is easy to fix -- just add back the finalAttrs inheritance of cargoHash.

JohnRTitor · 2025-03-03T06:18:51Z

@JohnRTitor For non-leaf packages, and especially far reaching changes like this, there should be a longer review period. Probably at least a week, since many contributors only have time for review e.g. during weekends.

Thanks, I will keep this in mind, though I thought this was a straightforward change.

JohnRTitor · 2025-03-03T07:19:02Z

pkgs/build-support/php/builders/v2/build-composer-project.nix

            patches
            pname
            src
-            vendorHash


I think this is intended? @drupol

Yes this was intended indeed. Is there any issue?

@drupol If you would like to restore the previous overriding behaviour, simply restore this line.

Yes it's done in the PR linked in this PR

drupol · 2025-03-03T07:22:23Z

For some unknown reasons, I received all the Github notifications from this thread now.

Let me know if there are adjustments to do and I'll gladly do it. So far, from the tests I made, I haven't encountered any issue.

ShamrockLee · 2025-03-03T08:14:32Z

Here's a minimal, Nix REPL-based reproducer that shows the changes of overriding behaviour.

After this PR:

nixpkgs_losslesscut on  HEAD (359c7c9) [$] 
❯ nix repl
Nix 2.24.10
Type :? for help.
nix-repl> lib = import ./lib                                           

nix-repl> pkgs = import ./. { config = { allowAliases = false; checkMeta = true; }; } 

nix-repl> pkgs.phpPackages.php-parallel-lint                           
«derivation /nix/store/ydvfkm3d61pmnwnnbm7xz44j73l19vwx-php-parallel-lint-1.4.0.drv»

nix-repl> pkgs.phpPackages.php-parallel-lint.vendorHash                
"sha256-ySdLlqlGKZ6LgmAOBMkBNoCAqWrgMwE/Cj6ZEPEsCko="

nix-repl> pkgs.phpPackages.php-parallel-lint.composerVendor            
«derivation /nix/store/zpshq5qc1nl6v17dhpbb27n844z32pli-php-parallel-lint-composer-vendor-1.4.0.drv»

nix-repl> pkgs.phpPackages.php-parallel-lint.composerVendor.outputHash 
"sha256-ySdLlqlGKZ6LgmAOBMkBNoCAqWrgMwE/Cj6ZEPEsCko="

nix-repl> (pkgs.phpPackages.php-parallel-lint.overrideAttrs { vendorHash = lib.fakeHash; })                           
«derivation /nix/store/61zgmy4zd55g65c880pv7znfm5p44mxc-php-parallel-lint-1.4.0.drv»

nix-repl> (pkgs.phpPackages.php-parallel-lint.overrideAttrs { vendorHash = lib.fakeHash; }).vendorHash
"sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="

nix-repl> (pkgs.phpPackages.php-parallel-lint.overrideAttrs { vendorHas
h = lib.fakeHash; }).composerVendor
«derivation /nix/store/zpshq5qc1nl6v17dhpbb27n844z32pli-php-parallel-lint-composer-vendor-1.4.0.drv»

nix-repl> (pkgs.phpPackages.php-parallel-lint.overrideAttrs { vendorHas
h = lib.fakeHash; }).composerVendor.outputHash
"sha256-ySdLlqlGKZ6LgmAOBMkBNoCAqWrgMwE/Cj6ZEPEsCko="

Before this PR:

nixpkgs_losslesscut on  HEAD (d857a85) [$] 
❯ nix repl
Nix 2.24.10
Type :? for help.
nix-repl> lib = import ./lib                                           

nix-repl> pkgs = import ./. { config = { allowAliases = false; checkMeta = true; }; }                                 

nix-repl> pkgs.phpPackages.php-parallel-lint                           
«derivation /nix/store/xdild3457dac1937f20lrzhnqq0546mg-php-parallel-lint-1.4.0.drv»

nix-repl> pkgs.phpPackages.php-parallel-lint.vendorHash                
"sha256-ySdLlqlGKZ6LgmAOBMkBNoCAqWrgMwE/Cj6ZEPEsCko="

nix-repl> pkgs.phpPackages.php-parallel-lint.composerVendor            
«derivation /nix/store/49k377wlvckalhqg71mba1ygmwd0k8n2-php-parallel-lint-composer-repository-1.4.0.drv»

nix-repl> pkgs.phpPackages.php-parallel-lint.composerVendor.outputHash 
"sha256-ySdLlqlGKZ6LgmAOBMkBNoCAqWrgMwE/Cj6ZEPEsCko="

nix-repl> (pkgs.phpPackages.php-parallel-lint.overrideAttrs { vendorHash = lib.fakeHash; })                           
«derivation /nix/store/7lg86qm6m9q97qdzls0hqn8sxmvvl08x-php-parallel-lint-1.4.0.drv»

nix-repl> (pkgs.phpPackages.php-parallel-lint.overrideAttrs { vendorHash = lib.fakeHash; }).vendorHash                
"sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="

nix-repl> (pkgs.phpPackages.php-parallel-lint.overrideAttrs { vendorHash = lib.fakeHash; }).composerVendor            
«derivation /nix/store/xn7pxq9djyrc8dcpqlg4kdm5i7fmskvx-php-parallel-lint-composer-repository-1.4.0.drv»

nix-repl> (pkgs.phpPackages.php-parallel-lint.overrideAttrs { vendorHash = lib.fakeHash; }).composerVendor.outputHash
"sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="



nixpkgs_losslesscut on  HEAD (d857a85) [$] took 48s 
❯

drupol · 2025-03-03T08:17:18Z

OK I see.

Then, with these changes introduced in this PR, there's no way to override the vendorHash any more?
If that's true, I'll update it tonight.

ShamrockLee · 2025-03-03T09:01:45Z

phpPackages.php-parallel-lint.overrideAttrs (
  previousAttrs:
  {
    composerVendor = previousAttrs.composerVendor.overrideAttrs (
      previousAttrs: {
        vendorHash = "new vendorHash";
      });
  })

and

phpPackages.php-parallel-lint.overrideAttrs (
  previousAttrs:
  {
    composerVendor = previousAttrs.composerVendor.overrideAttrs (
      previousAttrs: {
        outputHash = "new vendorHash";
      });
  })

still work.

The core issue is, which way of overriding is officially supported?

There should be one-- and preferably only one --obvious way to do it.

And it/its change require documentation.

drupol · 2025-03-03T09:02:48Z

OK.

I'll rework all of this tonight.

JohnRTitor · 2025-03-03T11:01:53Z

Do we want a revert in the meantime since this breaks existing behaviour?

drupol · 2025-03-03T11:02:59Z

I don't think it is necessary, tonight there will be a fix.

philiptaron · 2025-03-18T19:21:40Z

@drupol: Would you mind finding the right set of commits in this PR + #386757 to backport?

drupol · 2025-03-18T19:29:51Z

Aye, 't shall be done upon the morrow’s eve!

nixpkgs-ci · 2025-03-18T19:42:43Z

Successfully created backport PR for release-24.11:

[Backport release-24.11] build-support/php: support lib.extendMkDerivation #391058

drupol · 2025-03-18T20:10:50Z

@philiptaron: Hark! The request standeth fulfilled, sire!

philiptaron · 2025-03-18T20:13:30Z

@philiptaron: Hark! The request standeth fulfilled, sire!

Good sir, your promise of completion upon the morrows eve stands not just fulfilled, but over-fulfilled! What service, and dare I say, from what a fine fellow!

drupol · 2025-03-18T20:18:32Z

Good sir, thy word is but the wind that guideth mine humble keystrokes! What thou dost decree, I shall toil to fulfill with haste, lest I be cast into the abyss of unmerged commits and rebased sorrows! Speak, and it shall be wrought!

github-actions bot added the 6.topic: php PHP is a general-purpose scripting language geared towards web development. label Feb 28, 2025

drupol force-pushed the push-vnunotrsurnz branch from 678e601 to a2e2ee9 Compare February 28, 2025 16:21

github-actions bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Feb 28, 2025

drupol added 2 commits February 28, 2025 18:05

build-support/php: support lib.extendMkDerivation for `php.buildCom…

d9d0470

…poserProject2` Inspired from NixOS#382550 Context: NixOS#234651

build-support/php: support lib.extendMkDerivation for `php.mkCompos…

4cdddfa

…erVendor` Inspired from NixOS#382550 Context: NixOS#234651

drupol force-pushed the push-vnunotrsurnz branch from a2e2ee9 to 4cdddfa Compare February 28, 2025 17:06

drupol marked this pull request as ready for review February 28, 2025 17:48

JohnRTitor approved these changes Mar 1, 2025

View reviewed changes

JohnRTitor merged commit cd850d5 into NixOS:master Mar 1, 2025
57 of 59 checks passed

drupol deleted the push-vnunotrsurnz branch March 1, 2025 10:05

JohnRTitor reviewed Mar 3, 2025

View reviewed changes

drupol mentioned this pull request Mar 3, 2025

build-support/php: refactor build scripts #386757

Merged

13 tasks

philiptaron mentioned this pull request Mar 18, 2025

[Backport release-24.11] lib.extendMkDerivation: init #390950

Merged

7 tasks

drupol added the backport release-24.11 label Mar 18, 2025

nixpkgs-ci bot mentioned this pull request Mar 18, 2025

[Backport release-24.11] build-support/php: support lib.extendMkDerivation #391058

Merged

1 task

Uh oh!

Comments

Conversation

drupol commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

drupol commented Feb 28, 2025

Uh oh!

JohnRTitor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShamrockLee commented Mar 2, 2025

Uh oh!

jtojnar commented Mar 2, 2025

Uh oh!

ShamrockLee commented Mar 2, 2025

Uh oh!

JohnRTitor commented Mar 3, 2025

Uh oh!

JohnRTitor Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

drupol Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

ShamrockLee Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

drupol Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

drupol commented Mar 3, 2025

Uh oh!

ShamrockLee commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drupol commented Mar 3, 2025

Uh oh!

ShamrockLee commented Mar 3, 2025

Uh oh!

drupol commented Mar 3, 2025

Uh oh!

JohnRTitor commented Mar 3, 2025

Uh oh!

drupol commented Mar 3, 2025

Uh oh!

philiptaron commented Mar 18, 2025

Uh oh!

drupol commented Mar 18, 2025

Uh oh!

nixpkgs-ci bot commented Mar 18, 2025

Uh oh!

drupol commented Mar 18, 2025

Uh oh!

philiptaron commented Mar 18, 2025

Uh oh!

drupol commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

drupol commented Feb 28, 2025 •

edited

Loading

ShamrockLee commented Mar 3, 2025 •

edited

Loading