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

Fix #79026: Allow PHP_EXTRA_VERSION overrides #11706

Closed

Conversation

athos-ribeiro
Copy link
Contributor

When building from sources, someone distributing PHP may want to add a vendor specific string to the PHP_VERSION so users can differentiate multiple vendor builds from the same PHP version. For instance, a vendor backporting a bug fix to a no-longer-supported PHP version could extend their PHP_EXTRA_VERSION to allow their users to identify that they carry such fix by checking their PHP_VERSION.

@devnexen
Copy link
Member

Kind of make sense. cc @iluuu1994 @petk ?

Copy link
Member

@petk petk left a comment

Choose a reason for hiding this comment

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

Very nice idea. This is cool addition, yes. Thanks @athos-ribeiro

configure.ac Outdated Show resolved Hide resolved
@athos-ribeiro athos-ribeiro force-pushed the extra-version-from-env branch 2 times, most recently from 892669b to 4080e06 Compare July 15, 2023 16:37
@athos-ribeiro
Copy link
Contributor Author

Thanks!

@petk , I addressed your comments and re-based this on master. I was indeed wondering if it would be OK to declare it as a precious variable and ended up not doing it for consistency with the other vars around the same script.

Copy link
Member

@petk petk left a comment

Choose a reason for hiding this comment

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

@athos-ribeiro the dnl at the end of the line is to reduce empty newline in configure?

I think this is the most consistent way in this situation. It is available in the help output, variable is marked as precious (meaning it shouldn't be changed during the rest of the configure script) and we don't need to pass it to other macros because there won't be any substitution happening at this point.

This is ready to merge, if you're happy with the code style. Maybe just sync the space between the arguments of the two macro calls. Your call. Thank you for the patch.

When building from sources, someone distributing PHP may want to add a
vendor specific string to the PHP_VERSION so users can differentiate
multiple vendor builds from the same PHP version. For instance, a vendor
backporting a bug fix to a no-longer-supported PHP version could extend
their PHP_EXTRA_VERSION to allow their users to identify that they carry
such fix by checking their PHP_VERSION.
@athos-ribeiro
Copy link
Contributor Author

Thanks, @petk !

Yes, the dnl in the end of the line to to avoid generating an extra empty line in the final configure file.

I sync'd the style as you suggested and re-based this on master again.

+1 on merging this as is.

@petk
Copy link
Member

petk commented Jul 19, 2023

If it's fine with everyone, I'll merge this one also later today. I'll update the changelog... I think this is good addition and it resolves a bug.

@petk petk closed this in d35df89 Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants