php: Fully version the pear repository #26137

kabel · 2018-04-04T04:28:48Z

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

To more closely follow the Homebrew acceptable versioned formulae policies and to reduce issues with installing packages through pear/pecl, the pear repository has been separated for each version. The main pear packages (that are installed by upstream) will remain in the share/"pear" formula path, and be linked to HOMEBREW_PREFIX/share/pear on link, and should not interfere with anything else.

User's will retain the ability to use a single repository for all versions, via pear set-config.

This does not address any repositories that were already created and modified by previously running and pear/pecl maintenance commands, and users may be required to manually remove some files from share/"pear" formula path for any version they installed packages to.

~~I didn't bump the revision because it doesn't change the bottle/binaries. If we want people to get it without having to manually run brew post-install php, I can add in revisions.~~

This addresses #26108 and ensures fixes #25974.

ilovezfs · 2018-04-04T04:34:57Z

Formula/php.rb

@@ -70,6 +70,9 @@ def install
    ENV.append "CPPFLAGS", "-DU_USING_ICU_NAMESPACE=1"

    config_path = etc/"php/#{php_version}"
+    # Prevent system pear config from inhibitting pear install


If we're pouring a bottle, isn't this still a concern? Should we handle it in post_install?

No, It's only build time. post_install rebuilds it.

only one "t" in the word "inhibiting"

ilovezfs · 2018-04-04T04:40:56Z

I didn't bump the revision because it doesn't change the bottle/binaries. If we want people to get it without having to manually run brew post-install php, I can add in revisions.

yeah sounds like it's worth revision bumping

ilovezfs · 2018-04-04T05:43:07Z

Formula/php.rb

@@ -218,7 +222,7 @@ def post_install
    php_ext_dir = opt_prefix/"lib/php"/php_basename

    # fix pear config to install outside cellar
-    pear_path = HOMEBREW_PREFIX/"share/pear"
+    pear_path = HOMEBREW_PREFIX/"share/pear@#{php_version}"


Why do we need to version the main unversioned formula? That typically ends up meaning that we then need to provide automatic migration when e.g. this gets upgraded to 7.3.x

Wouldn't it mean the opposite as in if the files are attached to the version itself rather than the latest version it would make more sense ? Otherwise we would have to migrate when the version switches from 7.2 to 7.3 which potentially could break things.

The existing files stop working when php is upgraded?

You're probably right, I mixed it up with the compiled binaries but they are stored elsewhere. The pear directory only contains the pear php code so it should be reasonably portable between versions.

That's fine if we want the user controlled repository to be compatible with the next major/minor release--although we already have the issue that the versioned config php.ini doesn't migrate forward. Do you have a suggestion as the the suffix to use? The path without a suffix will always be the repository that upstream installs to make the pear and pecl commands work, and we don't want to conflict with the linked files.

SMillerDev · 2018-04-05T09:37:45Z

@kabel what's your opinion on ensuring that the extension_dir setting in php.ini is correct? (replacing it with the new path for people who only upgraded)

ilovezfs · 2018-04-05T10:20:36Z

That's fine if we want the user controlled repository to be compatible with the next major/minor release--although we already have the issue that the versioned config php.ini doesn't migrate forward. Do you have a suggestion as the the suffix to use? The path without a suffix will always be the repository that upstream installs to make the pear and pecl commands work, and we don't want to conflict with the linked files.

@kabel Can we relocate the linked files instead of linking them to avoid the redundancy? For example, we could stash them somewhere in pkgshare and then copy them over in post install. I think HOMEBREW_PREFIX/"share/pear" is nicer than HOMEBREW_PREFIX/"share/pear-latest"

kabel · 2018-04-06T01:46:19Z

what's your opinion on ensuring that the extension_dir setting in php.ini is correct? (replacing it with the new path for people who only upgraded)

I'd be okay with maybe putting a warning in post_install, but I'm leery of trying to manage user configuration.

MikeMcQuaid · 2018-04-07T06:27:14Z

Formula/php.rb

@@ -70,6 +71,9 @@ def install
    ENV.append "CPPFLAGS", "-DU_USING_ICU_NAMESPACE=1"

    config_path = etc/"php/#{php_version}"
+    # Prevent system pear config from inhibiting pear install
+    (config_path/"pear.conf").delete if (config_path/"pear.conf").exist?


I'd be wary of deleting user files here. Additionally, bear in mind this won't work for bottled installs.

I am a bit wary, but pear has a two-tier configuration system that provides a "system" config and a "user" config. Users should not be modifying the system config and the post_install will rebuild it. The guard is there specifically for source builds, as anyone that has this config file with configuration pointed at a pear repository where the pear files are already installed will skip the pear installation of binaries in the cellar. The would mean build from source upgrades would be missing the pear and pecl binaries.

ilovezfs · 2018-04-07T07:31:48Z

Formula/php.rb

@@ -219,6 +224,7 @@ def post_install

    # fix pear config to install outside cellar
    pear_path = HOMEBREW_PREFIX/"share/pear"
+    cp_r pkgshare/"pear/.", pear_path


Is this re-runnable?

It is. I've run it quite a few times in testing just to make sure that it didn't override manually installed things and keeps the upstream installed files in sync. Of course more robust testing from other users would be greatly appreciated (we all know what works4me gets).

westberliner · 2018-04-08T17:34:53Z

Tested this change request today. Worked for me. Hope, that it will be merged soon.

.

ilovezfs · 2018-04-11T15:43:14Z

@kabel #26298 is now merged. Please rebase this on master when you have a chance, and make it one commit per formula. Thanks!

A shared version is still possible with user configuration, but we should follow the versioned policies to avoid link problems between versions.

ilovezfs · 2018-04-12T06:23:56Z

Thanks @kabel 🚀

Introduced in Homebrew/homebrew-core#26137

ilovezfs reviewed Apr 4, 2018

View reviewed changes

ilovezfs added the php PHP use is a significant feature of the PR or issue label Apr 4, 2018

ilovezfs previously requested changes Apr 4, 2018

View reviewed changes

mmelvin0 mentioned this pull request Apr 5, 2018

php: Install PECL Modules for different PHP Versions #26108

Closed

MikeMcQuaid reviewed Apr 7, 2018

View reviewed changes

ilovezfs reviewed Apr 7, 2018

View reviewed changes

ilovezfs mentioned this pull request Apr 10, 2018

php: Add autoconf dependency #26298

Closed

4 tasks

Neodork mentioned this pull request Apr 11, 2018

PECL manager pear directory improvements weprovide/valet-plus#134

Closed

5 tasks

kabel added 4 commits April 11, 2018 20:34

php: Fully version pear repo

ce8cb19

A shared version is still possible with user configuration, but we should follow the versioned policies to avoid link problems between versions.

[email protected]: Fully version pear repo

bc87ad9

A shared version is still possible with user configuration, but we should follow the versioned policies to avoid link problems between versions.

[email protected]: Fully version pear repo

ea30433

A shared version is still possible with user configuration, but we should follow the versioned policies to avoid link problems between versions.

[email protected]: Fully version pear repo

2564749

A shared version is still possible with user configuration, but we should follow the versioned policies to avoid link problems between versions.

ilovezfs approved these changes Apr 12, 2018

View reviewed changes

ilovezfs closed this in 09dc59d Apr 12, 2018

ilovezfs mentioned this pull request Apr 12, 2018

Cannot brew link --force [email protected] out of the box #25974

Closed

kabel deleted the php-version-pear branch April 22, 2018 23:03

Neodork added a commit to tamtamnl/valet-plus that referenced this pull request Apr 26, 2018

Fix PECL switching directories per PHP version

cda9a99

Introduced in Homebrew/homebrew-core#26137

samgranger pushed a commit to weprovide/valet-plus that referenced this pull request Apr 30, 2018

Fix PECL switching directories per PHP version (#154)

81ac3c0

Introduced in Homebrew/homebrew-core#26137

Blackskyliner mentioned this pull request May 10, 2018

Warning: Use php instead of deprecated homebrew/php/php72 #26107

Closed

lock bot added the outdated PR was locked due to age label May 22, 2018

lock bot locked as resolved and limited conversation to collaborators May 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

php: Fully version the pear repository #26137

php: Fully version the pear repository #26137

kabel commented Apr 4, 2018 •

edited

Loading

ilovezfs Apr 4, 2018

kabel Apr 4, 2018

ilovezfs Apr 4, 2018

kabel Apr 4, 2018

ilovezfs commented Apr 4, 2018

ilovezfs Apr 4, 2018

javian Apr 4, 2018

ilovezfs Apr 4, 2018

javian Apr 4, 2018

kabel Apr 4, 2018

SMillerDev commented Apr 5, 2018

ilovezfs commented Apr 5, 2018

kabel commented Apr 6, 2018

MikeMcQuaid Apr 7, 2018

kabel Apr 7, 2018

ilovezfs Apr 7, 2018

kabel Apr 7, 2018

westberliner commented Apr 8, 2018

ilovezfs commented Apr 11, 2018

ilovezfs commented Apr 12, 2018

php: Fully version the pear repository #26137

php: Fully version the pear repository #26137

Conversation

kabel commented Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovezfs commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SMillerDev commented Apr 5, 2018

ilovezfs commented Apr 5, 2018

kabel commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westberliner commented Apr 8, 2018

ilovezfs commented Apr 11, 2018

ilovezfs commented Apr 12, 2018

kabel commented Apr 4, 2018 •

edited

Loading