Skip to content

Make configure compatible with PHP 7.4 #34

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

Merged
merged 12 commits into from
Mar 26, 2019
Merged

Conversation

ausi
Copy link
Contributor

@ausi ausi commented Mar 13, 2019

This is a followup to #29 using separate config files as requested by @BanzaiMan

@ausi
Copy link
Contributor Author

ausi commented Mar 13, 2019

The libonig issue still needs to be resolved I think.

#29 (comment)

the build is now likely to fail due to a missing libonig dependency.

@BanzaiMan To get an additional dependency, would it be sufficient to install libonig-dev in .travis.yml, or would this result in a missing shared object dependency when the built binary is used? That is, does something additional need to be done to make sure that the necessary SO is also present in the container/VM in some manner?

Copy link
Contributor

@joshk joshk left a comment

Choose a reason for hiding this comment

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

Due to the use of :

$TRAVIS_BUILD_DIR/default_configure_options.${RELEASE}

in the install files, and the use of default_configure_options.trusty-master style configure files, it might be best to use the custom_configure_options file and then add them to the default file used.

@nikic
Copy link
Contributor

nikic commented Mar 17, 2019

Why are you disabling PEAR on master? It should be compatible with PHP 8. At least the Console_GetOpt issue you're referencing was fixed a few weeks ago.

@joshk
Copy link
Contributor

joshk commented Mar 17, 2019

hey @nikic, I did lots of testing but it seems as though the version of Console_Getopt packaged in PEAR is old and does not include the each() fix.

@@ -0,0 +1,49 @@
--enable-intl
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this one also need all the version specific variations, at least for baseline, 7.4, master? (Which is also the reason why I still think that #29 is a more scalable solution to this problem.)

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, i borrowed this from the xenial base line default for my testing.

@nikic
Copy link
Contributor

nikic commented Mar 17, 2019

@joshk Just tested this locally and you are absolutely right. This must be a recent regression as this was working a few days ago :/ I'll try to get this fixed.

@joshk
Copy link
Contributor

joshk commented Mar 17, 2019

awesome, thanks @nikic
I spent a lot of time hunting it down and then seeing if I could build an updated release locally, but since PEAR needs PEAR to build PEAR, I was a bit stuck.

@joshk
Copy link
Contributor

joshk commented Mar 17, 2019

I have to admit that I hijacked this PR a little to test some build system improvements. Once this gets merged in I can create a PR with my tweaks.
(this will make it easier to build other OS versions)

@@ -7,7 +7,7 @@ travis_time_start

if [[ ! $VERSION =~ ^7 && ! $VERSION =~ ^master$ ]]; then
pecl download memcache-beta
tar zxvf memcache*.tgz && pushd memcache*
tar zxvf memcache*.tgz && pushd memcache*/
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these trailing slashes for?

@BanzaiMan BanzaiMan merged commit f3e0b44 into travis-ci:default Mar 26, 2019
@nikic
Copy link
Contributor

nikic commented Mar 26, 2019

Forgot to report back here, the go-pear.phar issue has been resolved (again...) so it should be fine to also install on master.

@nikic
Copy link
Contributor

nikic commented Mar 26, 2019

Also in the meantime --enable-maintainer-zts has become --enable-zts on master.

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