fix: configure manage libxml multiple include paths

[Antonov548]

Closes #1190

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

[krlmlr]

Thanks, looks good. I wonder if sed -r would give cleaner regular expressions. Is it worth renaming the xml2_include_dir variable?

[szhorvat]

Why do we need to keep the sed line to cut the -I flag? Isn't everything returned by xml2-config --cflags required to be added to the compilation command anyway? Note that linker flags won't be returned with --cflags—those are in xml2-config --libs instead.

Removing the fragile sed reduces the chance of things going wrong in the future.

[Antonov548]

Why do we need to keep the sed line to cut the -I flag? Isn't everything returned by xml2-config --cflags required to be added to the compilation command anyway? Note that linker flags won't be returned with --cflags—those are in xml2-config --libs instead.

Removing the fragile sed reduces the chance of things going wrong in the future.

As I remember the problem is that xml2-config --cflags returns -I/path without quotes. And if libxml library is installed in some folder with space in the name, then it become issue.

[szhorvat]

Also, to be completely robust, don't we need to add the output of xml2-config --libs to LDFLAGS? What we have now hard-codes these flags, but on some systems they might be different (depending on how libxml2 was configured there).

[szhorvat]

As I remember the problem is that xml2-config --cflags returns -I/path without quotes. And if libxml library is installed in some folder with space in the name, then it become issue.

Are you sure? Do we have an example of this? If yes, there should be a comment about this in the configure script.

[Antonov548]

As I remember the problem is that xml2-config --cflags returns -I/path without quotes. And if libxml library is installed in some folder with space in the name, then it become issue.

Are you sure? Do we have an example of this? If yes, there should be a comment about this in the configure script.

I think we had an issue. I will try to find...
I'm would be happy to fully remove it. I don't like this custom regex.

[szhorvat]

I just attempted to install libxml2 into a directory with spaces in the path, and the xml2-config script ended up being completely broken. It contained unquoted paths, and wouldn't run at all. So you're right, we shouldn't trust that this would work without issues.

[Antonov548]

I just attempted to install libxml2 into a directory with spaces in the path, and the xml2-config script ended up being completely broken. It contained unquoted paths, and wouldn't run at all. So you're right, we shouldn't trust that this would work without issues.

This is pull request in which I have added quotes. #1000

The problem was on Windows. But then I remember I also tried to make this trick on Ubuntu which you did and decided to add sanity check for Ubuntu as well.

[szhorvat]

If the problem was quoted paths with spaces, then wouldn't the regex 's/-I([^ ]*)/-I"\1"/g' be incorrect? Isn't this going up to the first space, which is exactly what needs to be quoted? We should have a concrete example of the problem that we're trying to fix, and include it in a comment ...

[Antonov548]

If the problem was quoted paths with spaces, then wouldn't the regex 's/-I([^ ]*)/-I"\1"/g' be incorrect? Isn't this going up to the first space, which is exactly what needs to be quoted? We should have a concrete example of the problem that we're trying to fix, and include it in a comment ...

Oh, yes, You are right.
This regex should be more smart. I not very good at regex. I guess it should take into account position of -I.

[szhorvat]

Given that the bug is in xml2-config, IMO we shouldn't try to fix it. It's not really our problem, and it's not really possible to fix it properly, since we don't know where to stop parsing the path ... There is a risk that by trying too hard, we will make a bigger mess, or break something that wasn't broken before.

We should tell people not to install RTools in C:\Program Files as a workaround.

Opinion, @krlmlr ?

[szhorvat]

I guess it should take into account position of -I.

I mean, do we parse up to the next -I? What if the next flag is a -D, not an -I? It's plausible that this could happen. Do we parse up to the next -? What if the path has a - character? On Windows, it's common to have the real name be the username, and real names tend to contain both spaces and dashes. If someone installs RTools into their home directory, we can easily hit a dash.

This is why I'm suggesting NOT to fix this, as it's not possible to create a robust fix on our end.

After all there's an easy workaround: Install RTools to a safe location, without weird characters in the path.

[szhorvat]

For safety, it'd be good to check what the default installation location is for RTools. If that is not in Program Files then we're good.

[Antonov548]

I guess it should take into account position of -I.

I mean, do we parse up to the next -I? What if the next flag is a -D, not an -I? It's plausible that this could happen. Do we parse up to the next -? What if the path has a - character? On Windows, it's common to have the real name be the username, and real names tend to contain both spaces and dashes. If someone installs RTools into their home directory, we can easily hit a dash.

This is why I'm suggesting NOT to fix this, as it's not possible to create a robust fix on our end.

After all there's an easy workaround: Install RTools to a safe location, without weird characters in the path.

I'm agree to leave as it is, I mean just what xml/pkg-config returns.
As I said the issue was really with Windows which not related to this changes.

[szhorvat]

The RTools installation instructions have this:

Rtools43 may be installed from the Rtools43 installer. It is recommended to use the defaults, including the default installation location of C:\rtools43.

We have a strong case. We could just add a troubleshooting entry about this for people who chose to install to a different location. Most people don't compile on Windows anyway, so the impact is small.

[krlmlr]

This script isn't run on Windows, AFAICT. Windows runs only configure.ucrt or configure.win , the latter is empty for us, the former is missing.

No strong opinion. Can we somehow fix upstream in xml2-config ? Is it also broken for pkgconfig ?

[szhorvat]

This script isn't run on Windows, AFAICT

Then this workaround isn't needed at all—let's remove it completely.

Can we somehow fix upstream in xml2-config ?

If xml2-config is not used on Windows, then there's no reason to bother with this. On Unix, no one installs software in locations with spaces in the path. When I tried to install libxml2 to such a location, I got a non-functioning xml2-config. So this issue will never reach igraph on Unix-like systems.

[szhorvat]

Can we please rename xml2_include_paths to xml2_cflags to better reflect what it is?

Shall we fix the link flags (PKG_LIBS) as well? They shouldn't be hard-coded. They should be taken from xml2-config --libs or pkg-config --libs libxml-2.0.

[szhorvat]

@krlmlr It looks like fixing all of phoenix will take longer, but if just this one PR could be included in a quick-release, in its current state, that would probably cut the number of support requests in half.

The next most impactful fix would be making dependencies optional again, #1156, but that is a lot more work so let's maybe not wait for it before other essential fixes go out ...

[krlmlr]

Thanks! Opening a follow-up issue to handle Szabolcs's feedback