Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build-support/php: add more passthru attributes #305256

Closed
wants to merge 2 commits into from
Closed

build-support/php: add more passthru attributes #305256

wants to merge 2 commits into from

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Apr 19, 2024

Related to #302136 (comment)

Description of changes

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:
  • 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.05 Release Notes (or backporting 23.05 and 23.11 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.

@drupol drupol marked this pull request as ready for review April 19, 2024 07:37
@drupol drupol mentioned this pull request Apr 19, 2024
Merged
13 tasks
@drupol drupol force-pushed the build-support/php/add-more-passthru-attrs branch from 3cc27af to 1bcc5fc Compare April 19, 2024 08:16
@drupol drupol force-pushed the build-support/php/add-more-passthru-attrs branch from 1bcc5fc to a8387fd Compare April 19, 2024 09:59
jtojnar
jtojnar reviewed Apr 19, 2024
View reviewed changes
pkgs/build-support/php/build-composer-project.nix Outdated
@@ -66,13 +66,18 @@ let
inherit composer composer-local-repo-plugin;
inherit (finalAttrs) patches pname src vendorHash version;

php = phpDrv;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not this already be the default thanks to using phpDrv.mkComposerRepository?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unclear to me yet, I was awaiting from you to confirm this. I'm a bit lost with those finalAttrs and previousAttrs. Can you help?

Copy link
Member

@jtojnar jtojnar Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should be the default. php passes it to the php.packages scope:

nixpkgs/pkgs/development/interpreters/php/generic.nix

Line 92 in 70721b6

phpPackage = phpWithExtensions;

which sets it as the php attribute:

nixpkgs/pkgs/top-level/php-packages.nix

Line 181 in ee5ad61

php = phpPackage;

and scope’s callPackage will pass it to buildComposerPackage:

nixpkgs/pkgs/top-level/php-packages.nix

Line 57 in ee5ad61

buildComposerProject = callPackage ../build-support/php/build-composer-project.nix { };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for passing it to buildComposerProject = callPackage ../build-support/php/build-composer-project.nix { }; Thanks!

But when it comes to mkComposerRepository, shouldn't it be inherited from buildComposerProject ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be, since if you override php in buildComposerProject, the finalAttrs.php will reflect that and you are taking mkComposerRepository from its scope:

nixpkgs/pkgs/development/interpreters/php/generic.nix

Line 91 in 70721b6

php-packages = (callPackage ../../../top-level/php-packages.nix {

nixpkgs/pkgs/development/interpreters/php/generic.nix

Line 167 in 70721b6

inherit (php-packages) extensions buildPecl mkComposerRepository buildComposerProject composerHooks mkExtension;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK Good to know.

@drupol drupol force-pushed the build-support/php/add-more-passthru-attrs branch from a8387fd to b67c751 Compare April 19, 2024 10:25
@drupol drupol force-pushed the build-support/php/add-more-passthru-attrs branch from b67c751 to b07ab2e Compare April 19, 2024 10:43
jtojnar
jtojnar reviewed Apr 19, 2024
View reviewed changes
pkgs/build-support/php/build-composer-project.nix Outdated
composerLock = previousAttrs.composerLock or null;
composerNoDev = previousAttrs.composerNoDev or true;
composerNoPlugins = previousAttrs.composerNoPlugins or true;
composerNoScripts = previousAttrs.composerNoScripts or true;
composerStrictValidation = previousAttrs.composerStrictValidation or true;
};

passthru = (previousAttrs.passthru or {}) // {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this will shadow php externally:

nix-repl> (stdenv.mkDerivation {name = "foo"; php = 1; passthru.php = 2;}).php
2

nix-repl> (stdenv.mkDerivation (finalAttrs: {name = "foo"; php = 1; passthru.php = 2; c = finalAttrs.php;})).c 
1

We should probably drop this and do stdenvNoCC.mkDerivation ({ inherit php; } // args) to serve as an initial value and replacing phpDrv with finalAttrs.php instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like the following (untested):

--- a/pkgs/build-support/php/build-composer-project.nix
+++ b/pkgs/build-support/php/build-composer-project.nix
@@ -4,8 +4,10 @@ let
   buildComposerProjectOverride = finalAttrs: previousAttrs:
 
     let
-      phpDrv = finalAttrs.php or php;
-      composer = finalAttrs.composer or phpDrv.packages.composer;
+      initialArgs = {
+        inherit php;
+      };
+      composer = finalAttrs.composer or finalAttrs.php.composer;
       composer-local-repo-plugin = callPackage ./pkgs/composer-local-repo-plugin.nix { };
     in
     {
@@ -18,12 +20,12 @@ let
       nativeBuildInputs = (previousAttrs.nativeBuildInputs or [ ]) ++ [
         composer
         composer-local-repo-plugin
-        phpDrv
-        phpDrv.composerHooks.composerInstallHook
+        finalAttrs.php
+        finalAttrs.php.composerHooks.composerInstallHook
       ];
 
       buildInputs = (previousAttrs.buildInputs or [ ]) ++ [
-        phpDrv
+        finalAttrs.php
       ];
 
       patches = previousAttrs.patches or [ ];
@@ -62,7 +64,7 @@ let
         runHook postInstallCheck
       '';
 
-      composerRepository = phpDrv.mkComposerRepository {
+      composerRepository = finalAttrs.php.mkComposerRepository {
         inherit composer composer-local-repo-plugin;
         inherit (finalAttrs) patches pname src vendorHash version;
 
@@ -82,4 +84,4 @@ let
       };
     };
 in
-args: (stdenvNoCC.mkDerivation args).overrideAttrs buildComposerProjectOverride
+args: (stdenvNoCC.mkDerivation (initialArgs // args)).overrideAttrs buildComposerProjectOverride

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialArgs is not in the scope in stdenvNoCC, I moved it one level up... but it's not working anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what I can do to move on on this PR

@drupol drupol marked this pull request as draft April 19, 2024 11:03
@drupol drupol force-pushed the build-support/php/add-more-passthru-attrs branch from 2d42f64 to 7d620b9 Compare April 19, 2024 11:06
@drupol drupol force-pushed the build-support/php/add-more-passthru-attrs branch from 7d620b9 to d223bbc Compare April 19, 2024 11:35
@drupol
Copy link
Contributor Author

drupol commented Jul 29, 2024

No feedback in time and loosing interests into this. Closing.

@drupol drupol closed this Jul 29, 2024
@drupol drupol deleted the build-support/php/add-more-passthru-attrs branch July 29, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@aanderse aanderse Awaiting requested review from aanderse

@globin globin Awaiting requested review from globin

@Ma27 Ma27 Awaiting requested review from Ma27

@talyz talyz Awaiting requested review from talyz

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: php 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drupol @jtojnar @wegank