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

php73: fix builds on darwin architecture #201

Merged
merged 1 commit into from
Jan 25, 2023
Merged

php73: fix builds on darwin architecture #201

merged 1 commit into from
Jan 25, 2023

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Jan 20, 2023

This PR:

@HelloWorld017
Copy link
Contributor

I'll check whether this builds well on my machine

@HelloWorld017
Copy link
Contributor

HelloWorld017 commented Jan 20, 2023

This derivation also keeps failing with following issue:

PEAR package PHP_Archive not installed: generated phar will require PHP's phar extension be enabled.

The flag --with-pcre-regex=${prev.pcre2.dev} should be added in order to the php73 to be built.
Here's the commit:

$ git am - <<'EOF'
From ac13f9d341dc63bf366699cfc2911601b73a849b Mon Sep 17 00:00:00 2001
From: nenw <[email protected]>
Date: Fri, 20 Jan 2023 20:00:17 +0900
Subject: [PATCH] php73: fix pcre config flags

---
 pkgs/phps.nix | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pkgs/phps.nix b/pkgs/phps.nix
index 71a884d..760397e 100644
--- a/pkgs/phps.nix
+++ b/pkgs/phps.nix
@@ -43,6 +43,7 @@ let
 
               "--enable-libxml"
               "--with-libxml-dir=${prev.libxml2.dev}"
+              "--with-pcre-regex=${prev.pcre2.dev}"
             ];
 
           buildInputs =
-- 
2.38.1

EOF

@drupol drupol force-pushed the php73/fix-builds branch 2 times, most recently from e5e7735 to ba7bf9c Compare January 20, 2023 16:27
@drupol
Copy link
Collaborator Author

drupol commented Jan 20, 2023

Can you try again now?

pkgs/php/7.3.nix Outdated Show resolved Hide resolved
@drupol drupol force-pushed the php73/fix-builds branch 5 times, most recently from b3af256 to 0f749b8 Compare January 20, 2023 22:01
@drupol
Copy link
Collaborator Author

drupol commented Jan 20, 2023

I pushed a temporary commit (part of #85) to check if it is correctly building on MacOS.
I'll remove it when this PR has been approved, just before merging it.

@drupol
Copy link
Collaborator Author

drupol commented Jan 25, 2023

@HelloWorld017 @shyim Any update here?

@shyim
Copy link
Contributor

shyim commented Jan 25, 2023

shyim in nix-phps on  php73/fix-builds [?] took 5m47s
❯ ./result/bin/php -v
PHP 7.3.28 (cli) (built: Jan 25 2023 08:29:17) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.28, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.3.28, Copyright (c) 1999-2018, by Zend Technologies

whooo works

@HelloWorld017
Copy link
Contributor

Yes, it works on my laptop too. 👍

@drupol
Copy link
Collaborator Author

drupol commented Jan 25, 2023

@jtojnar Should I remove the commit 2ca5c87 before merging? WDYT ?

@jtojnar
Copy link
Member

jtojnar commented Jan 25, 2023

Might be faster to merge it that way, since we would have to review that patch otherwise.

@drupol
Copy link
Collaborator Author

drupol commented Jan 25, 2023

I just removed the commit.

@drupol drupol merged commit 7e53d72 into master Jan 25, 2023
@drupol drupol deleted the php73/fix-builds branch January 25, 2023 22:17
@@ -43,6 +43,9 @@ let

"--enable-libxml"
"--with-libxml-dir=${prev.libxml2.dev}"
]
++ prev.lib.optionals (prev.lib.versionAtLeast args.version "7.3") [
"--with-pcre-regex=${prev.pcre2.dev}"
Copy link
Member

Choose a reason for hiding this comment

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

Actually, not sure about this. Should not it be versionOlder args.version "7.4"? I would expect newer PHP versions to pick it up using pkg-config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure either, maybe @shyim or @HelloWorld017 knows?

Copy link
Contributor

@HelloWorld017 HelloWorld017 Jan 26, 2023

Choose a reason for hiding this comment

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

He is right. It should be applied to php<=7.3 (7.3, 7.2, ...)

You can just add it below to the xml line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dammit...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll open a new PR right now. Sorry about that.

drupol added a commit that referenced this pull request Jan 26, 2023
drupol added a commit that referenced this pull request Jan 26, 2023
drupol added a commit that referenced this pull request Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants