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

php: Fix php pcre by using external lib #31526

Merged
merged 5 commits into from Nov 13, 2017
Merged

php: Fix php pcre by using external lib #31526

merged 5 commits into from Nov 13, 2017

Conversation

srhb
Copy link
Contributor

@srhb srhb commented Nov 11, 2017 •
edited

Motivation for this change

Fixes #31451
Fixes #31499

Things done

Tested using https://gist.github.com/srhb/2d9f5354cdf16c96ec3356f4c5383303

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@srhb srhb mentioned this pull request Nov 11, 2017
Closed
8 tasks
@srhb
Copy link
Contributor Author

srhb commented Nov 11, 2017

Alternative candidate for the fix: #31525

@srhb
Copy link
Contributor Author

srhb commented Nov 12, 2017 •
edited

@vcunat is the only one who has voiced their opinion so far, preferring using the external pcre library (which I'm leaning towards as well.)

I've added a NixOS test that blocks the channel advancement (I think?) that ensures similar issues don't arise again without us noticing. I could use a review on this, since I'm new to messing with the release files. :)

@vcunat
Copy link
Member

vcunat commented Nov 12, 2017

This should block the big channels and not the small ones. Given that we do have php in the small ones as well, I suggest to make them depend on this check as well (better confirmed by someone else as well).

orivej
orivej reviewed Nov 12, 2017
View reviewed changes
pkgs/development/interpreters/php/default.nix Outdated
@@ -91,6 +91,12 @@ let
configureFlags = [ "--enable-pcntl" ];
};

# pcre functionality is tested in nixos/tests/php-pcre.nix
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be more noticeable to say at the top of the file that there is nixos php test available.

@orivej
Copy link
Contributor

orivej commented Nov 12, 2017

I agree with @vcunat that we should use external pcre, both because unbundling is more maintanable and because the other approach as it is implemented in php is fragile. It bothers me that you add pcreExternal option, whereas we won't support it being off unless we merge #31525 too, but I suppose we can leave it hoping that no one will use it without a good reason.

@srhb
Copy link
Contributor Author

srhb commented Nov 12, 2017

@orivej That is a good point. I can simply set it to true instead, but is there another way you would prefer?

@srhb
Copy link
Contributor Author

srhb commented Nov 12, 2017

@orivej How about 7e17685 ?

@orivej
Copy link
Contributor

orivej commented Nov 12, 2017

Thank you.

@orivej orivej merged commit b62ad4f into NixOS:master Nov 13, 2017
orivej added a commit that referenced this pull request Nov 13, 2017
@orivej
php: Fix php pcre by using external lib
b76e7f8 
Merge pull request #31526 from srhb/fix-php-external-pcre

Since #30963 (bbb6ca7 on release-17.09) regex
subgroup matches in mod_php were returning incorrect results due to symbol
conflicts between system pcre used by Apache and pcre build into php.

(cherry picked from commit b62ad4f)
@orivej
Copy link
Contributor

orivej commented Nov 13, 2017

Released in b76e7f8

@orivej orivej added the 8.has: port to stable A PR already has a backport to the stable release. label Nov 13, 2017
orivej added a commit that referenced this pull request Nov 15, 2017
@orivej
phpPackages.apcu,imagick: fix build after #31526
8562538
orivej added a commit that referenced this pull request Nov 15, 2017
@orivej
phpPackages.apcu,imagick: fix build after #31526
50d4178 
(cherry picked from commit 8562538)
orivej added a commit that referenced this pull request Nov 15, 2017
@orivej
php56Packages.apcu40: fix build after #31526
2fc2063
orivej added a commit that referenced this pull request Nov 15, 2017
@orivej
php56Packages.apcu40: fix build after #31526
b0e50cc 
(cherry picked from commit 2fc2063)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orivej orivej orivej left review comments

Assignees
No one assigned
Labels
8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-darwin: 11-100 10.rebuild-linux: 0 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@srhb @vcunat @orivej @GrahamcOfBorg