build-support/php: support `lib.extendMkDerivation` #385830

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

          inherit (finalAttrs)
            patches
            pname
            src
-            vendorHash


I think this is intended? @drupol

Yes this was intended indeed. Is there any issue?

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.

github-actions bot added the 6.topic: php 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 10.rebuild-linux: 11-100 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build-support/php: support `lib.extendMkDerivation` #385830

build-support/php: support `lib.extendMkDerivation` #385830

drupol commented Feb 28, 2025 •

edited

Loading

drupol commented Feb 28, 2025

JohnRTitor left a comment

ShamrockLee commented Mar 2, 2025

jtojnar commented Mar 2, 2025

ShamrockLee commented Mar 2, 2025

JohnRTitor commented Mar 3, 2025

JohnRTitor Mar 3, 2025

drupol Mar 3, 2025

drupol commented Mar 3, 2025

ShamrockLee commented Mar 3, 2025 •

edited

Loading

drupol commented Mar 3, 2025

ShamrockLee commented Mar 3, 2025

drupol commented Mar 3, 2025

JohnRTitor commented Mar 3, 2025

drupol commented Mar 3, 2025

build-support/php: support lib.extendMkDerivation #385830

build-support/php: support lib.extendMkDerivation #385830

Conversation

drupol commented Feb 28, 2025 • edited Loading

Things done

drupol commented Feb 28, 2025

JohnRTitor left a comment

Choose a reason for hiding this comment

ShamrockLee commented Mar 2, 2025

jtojnar commented Mar 2, 2025

ShamrockLee commented Mar 2, 2025

JohnRTitor commented Mar 3, 2025

JohnRTitor Mar 3, 2025

Choose a reason for hiding this comment

drupol Mar 3, 2025

Choose a reason for hiding this comment

drupol commented Mar 3, 2025

ShamrockLee commented Mar 3, 2025 • edited Loading

drupol commented Mar 3, 2025

ShamrockLee commented Mar 3, 2025

drupol commented Mar 3, 2025

JohnRTitor commented Mar 3, 2025

drupol commented Mar 3, 2025

build-support/php: support `lib.extendMkDerivation` #385830

build-support/php: support `lib.extendMkDerivation` #385830

drupol commented Feb 28, 2025 •

edited

Loading

ShamrockLee commented Mar 3, 2025 •

edited

Loading