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.

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

php: enable nts flags by default #194172

Merged
merged 1 commit into from
Nov 13, 2022
Merged

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Oct 3, 2022

This PR:

Context:

Currently, to build a NTS version of PHP in Nix, 2 flags needs to be updated as such:

ntsFlags = {
  apxs2Support = false;
  ztsSupport = false;
};

ntsPhpDrv = pkgs.php.override ntsFlags;

In the PR, only one flag is updated because the second flag depends on the first one.

This means that PHP will be built in ZTS on darwin and NTS on the other platforms. Is this what we want?
IMHO, I would prefer to not do that and I'll gladly updated this PR based on the maintainers feedback.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@aanderse
Copy link
Member

aanderse commented Oct 7, 2022

Isn't apxs2Support necessary for mod_php?

@mweinelt
Copy link
Member

Friendly ping, only 10 more days until branch-off.

@etu
Copy link
Contributor

etu commented Nov 12, 2022

I did some thinking @drupol, what do you think about this as the full set of changes?

This should also address the usage of apache @aanderse.

diff --git a/nixos/modules/services/web-servers/apache-httpd/default.nix b/nixos/modules/services/web-servers/apache-httpd/default.nix
index 3ccff8aa500..ed1426f52fb 100644
--- a/nixos/modules/services/web-servers/apache-httpd/default.nix
+++ b/nixos/modules/services/web-servers/apache-httpd/default.nix
@@ -18,7 +18,7 @@ let
     sed -i $out/bin/apachectl -e 's|$HTTPD -t|$HTTPD -t -f /etc/httpd/httpd.conf|'
   '';

-  php = cfg.phpPackage.override { apacheHttpd = pkg; };
+  php = cfg.phpPackage.override { apxs2Support = true; apacheHttpd = pkg; };

   phpModuleName = let
     majorVersion = lib.versions.major (lib.getVersion php);
diff --git a/pkgs/development/interpreters/php/generic.nix b/pkgs/development/interpreters/php/generic.nix
index d1b7c682955..e252e011105 100644
--- a/pkgs/development/interpreters/php/generic.nix
+++ b/pkgs/development/interpreters/php/generic.nix
@@ -43,7 +43,7 @@ let
     , phpdbgSupport ? true

       # Misc flags
-    , apxs2Support ? !stdenv.isDarwin
+    , apxs2Support ? false
     , argon2Support ? true
     , cgotoSupport ? false
     , embedSupport ? false

@aanderse
Copy link
Member

In combination with what @etu said I think we should add something to passthru indicating the php package is multithreaded. In the httpd module we can check phpPackage and add a warnings if this flag is present. Since none of our php packages are multithreaded this will be nicely backwards compatible.

@drupol
Copy link
Contributor Author

drupol commented Nov 12, 2022

In combination with what @etu said I think we should add something to passthru indicating the php package is multithreaded. In the httpd module we can check phpPackage and add a warnings if this flag is present. Since none of our php packages are multithreaded this will be nicely backwards compatible.

I'm all hears! Please give me some directions and I'll implement it.

@aanderse
Copy link
Member

Hmm... actually I misunderstood something about the httpd module. Maybe what I said isn't necessary... this is what I had in mind.

@etu
Copy link
Contributor

etu commented Nov 13, 2022

Hmm... actually I misunderstood something about the httpd module. Maybe what I said isn't necessary... this is what I had in mind.

First I thought "wait, how does that work? I didn't know one could read attributes sent in like that". It works because we already have the passthrou in place: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/interpreters/php/generic.nix#L312

I think that assertion @aanderse looks great, we should include that as well.

@drupol
Copy link
Contributor Author

drupol commented Nov 13, 2022

Just added the @aanderse suggestion.

@etu
Copy link
Contributor

etu commented Nov 13, 2022

Just triggering the build for PHP80 as well 🙂

@ofborg build php80 php81

@etu
Copy link
Contributor

etu commented Nov 13, 2022

@ofborg build php80.passthru.tests php81.passthru.tests

@etu etu merged commit 04dea74 into NixOS:master Nov 13, 2022
@drupol drupol deleted the php/enable-nts-by-default branch November 13, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants