Skip to content

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