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: Add autoconf dependency #26298

Closed
wants to merge 4 commits into from
Closed

Conversation

javian
Copy link
Contributor

@javian javian commented Apr 7, 2018

  • 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>)?

It was discovered that the autoconf dependency was missing as pointed out in #26170 which also made me realise we depend heavily on pecl but its not tested. I'm aware that only the php formula has the test at the moment but I'll add the others once we can confirm that this is the way forward.

Copy link
Contributor

@kabel kabel left a comment

Choose a reason for hiding this comment

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

Should probably depend on #26137. The test output also makes me think I should add the channel-update calls to post_install.

Formula/php.rb Outdated
@@ -289,6 +290,8 @@ def plist; <<~EOS
system "#{sbin}/php-fpm", "-t"
system "#{bin}/phpdbg", "-V"
system "#{bin}/php-cgi", "-m"
system "#{bin}/pecl", "install", "xdebug"
Copy link
Contributor

Choose a reason for hiding this comment

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

The sandbox isn't going to let you write here.

Copy link
Contributor Author

@javian javian Apr 9, 2018

Choose a reason for hiding this comment

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

Yeah I noticed that. I thought I could get around it by using config-set extension_dir but it didn't seem to work when I tried it. Haven't looked at the pecl code but I'm guessing the settings comes from phpize/php-config.

@javian
Copy link
Contributor Author

javian commented Apr 10, 2018

I removed the test for now since it requires some more work whereas the Autoconf addition is more straightforward.

@javian javian changed the title php: Add autoconf dependency and an automated test for pecl php: Add autoconf dependency Apr 10, 2018
@ilovezfs
Copy link
Contributor

And we're sure automake and libtool are not needed?

@ilovezfs
Copy link
Contributor

(I see libtool is already an indirect dep via unixodbc)

@fxcoudert
Copy link
Member

@ilovezfs doesn't the new dependency necessitate revision bumps?

@ilovezfs
Copy link
Contributor

No, we're doing revision bumps on #26137 so that will take care of it.

@javian
Copy link
Contributor Author

javian commented Apr 10, 2018

According to the PHP docs automake and libtool are required dependencies . http://php.net/manual/en/install.unix.php so you are correct. Should I add them as well ?

@kabel
Copy link
Contributor

kabel commented Apr 10, 2018

They're only necessary for building from a code repository checkout. Building from a release tarball and running pecl probably shouldn't need them.

@javian
Copy link
Contributor Author

javian commented Apr 11, 2018

@kabel right, that rings a bell now when we switched back to using php.net releases from the github ones.

@ilovezfs ilovezfs closed this in 70105f1 Apr 11, 2018
@ilovezfs
Copy link
Contributor

Thanks again @javian!

@lock lock bot added the outdated PR was locked due to age label May 11, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants