Skip to content

Fix #75871 - If pkg-config is available use it for libxml2 #3044

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

Closed
wants to merge 1 commit into from

Conversation

pmmaga
Copy link
Contributor

@pmmaga pmmaga commented Jan 25, 2018

Link for bugsnet: https://bugs.php.net/bug.php?id=75871

This removes the dependency on xml2-config but keeps it as a fallback in case pkg-config is not available.

@Majkl578
Copy link
Contributor

[Bug report author here] I can confirm this works with both the new version without xml2-config and the previous one with it. 👍

@weltling
Copy link
Contributor

Thanks for the PR. Perhaps it would be more correct to keep xml2-config as default. I was just comparing the outputs, fe on OpenSUSE i see

$ xml2-config --libs
-lxml2 -L/lib64 -lz -llzma -lm -ldl
$ pkg-config --libs libxml-2.0
-lxml2

Given that's only Debian for now, and only experimental, seems the other way round, to keep xml2-config and fallback to pkg-config, would be more backward compatible.

Thanks.

@pmmaga pmmaga changed the base branch from master to PHP-7.1 February 1, 2018 20:19
@pmmaga
Copy link
Contributor Author

pmmaga commented Feb 1, 2018

Thanks for the feedback! I've switched the precedence so xml2-config is still used when present.

@pmmaga
Copy link
Contributor Author

pmmaga commented Feb 6, 2018

Reverted trailing white-space changes

@weltling
Copy link
Contributor

weltling commented Feb 7, 2018

@pmmaga one question yet - any particular reason to target 7.1? Experimental might land or not, actually. If it went into unstable, we could put it, but otherwise master seems a good target for us. What would you say?

Thanks.

@pmmaga
Copy link
Contributor Author

pmmaga commented Feb 7, 2018

As it was a bug report I just followed the standard: "lowest actively supported branch that is affected". Given the way it is now implemented, nothing will change for people that have xml2-config available, so I'd say it's safe to support this from 7.1+. If anyway you'd prefer to see this only on master I also understand the reasoning, so both ways are fine for me.

@krakjoe
Copy link
Member

krakjoe commented Feb 8, 2018

@weltling I think I'm happy to merge into 7.1 if the default behaviour is retained, okay ?

@weltling
Copy link
Contributor

weltling commented Feb 8, 2018

@krakjoe ack, please do. Of course, one can see it separate from the Debian experimental.

Thanks.

@krakjoe
Copy link
Member

krakjoe commented Feb 8, 2018

Merged 5673c64

Thanks.

@krakjoe krakjoe closed this Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants