cmake: correctly detect if system provides regcomp - #5007
Conversation
We assume that if we are on Win32, Amiga OS, Solaris or SunOS, that the regcomp(3P) function cannot be provided by the system. Thus we will in these cases always include our own, bundled regex sources to make a regcomp implementation available. This test is obviously very fragile, and we have seen it fail on MSYS2/MinGW systems, which do in fact provide the regcomp symbol. The effect is that during compilation, we will use the "regex.h" header provided by MinGW, but use symbols provided by ourselves. This in fact may cause subtle memory layout issues, as the structure made available via MinGW doesn't match what our bundled code expects. There's one more problem with our regex detection: on the listed platforms, we will incorrectly include the bundled regex code even in case where the system provides regcomp_l(3), but it will never be used for anything. Fix the issue by improving our regcomp detection code. Instead of relying on a fragile listing of platforms, we can just use `CHECK_FUNCTION_EXISTS` instead. This will not in fact avoid the header-ordering problem. But we can assume that as soon as a system-provided "regex.h" header is provided, that `CHECK_FUNCTION_EXISTS` will now correctly find the desired symbol and thus not include our bundled regex code.
|
This makes sense to me. If @carrotIndustries gives a 👍 then I agree that we should merge this and ship a point release. |
|
I'm not quite sure wheter this PR has the desired effect. cmake output: Compiler invocation: Since gcc now picks the our |
|
It doesn't. My assumption was that if "regcomp.h" exists in MinGW, that there'd also be a So now it only works accidentally because I changed the order in which headers were added. The proposed fix is the right thing to do regardless of that. But in addition, we should probably also do what you proposed: instead of using |
On mingw,
|
|
Huh, interesting. I don't really feel like we should check for any weird package providing the functionality we search for. I'm a bit on the edge. The way it is now it's very fragile, but I'd say that to be true of the way includes are processed in general. This is more of a systemic failure of how the C preprocessor works. So going for "-I" instead of "-isystem" sounds like the right thing to do. But as I said earlier, I don't really like that solution -- I don't want to get warnings about our bundled dependencies. By the way, "/C/msys64/mingw64/include" looks to me like a standard includes path that is likely to already be present in the default search path of your mingw64 compiler. Do you have any idea why we explicitly add it? |
No idea, probably something cmake does. One option to make the build process less fragile is to rename the bundled regex header to something less generic than |
|
bump any ideas on how to move forward on this issue? |
When linking against bundled libraries, we include their header directories by using "-isystem". The reason for that is that we want to handle our vendored library headers specially, most importantly to ignore warnings generated by including them. By using "-isystem", though, we screw up the order of searched include directories by moving those bundled dependencies towards the end of the lookup order. Like this, chances are high that any other specified include directory contains a file that collides with the actual desired include file. Fix this by not treating the bundled dependencies' include directories as system includes. This will move them to the front of the lookup order and thus cause them to override system-provided headers. While this may cause the compiler to generate additional warnings when processing bundled headers, this is a tradeoff we should make regardless to fix builds on systems hitting this issue.
|
Thanks for your bump and sorry that I didn't have another look earlier. I've pushed an additional fix that switches over our build system to use "-I" instead of "-isystem" for bundled dependencies. While this is not perfect (e.g. additional warnings may get generated by the compiler due to that), I'd argue that it's the right thing to do. |
|
That fixed it for me, so feel free to merge. |
|
On Fri, Apr 26, 2019 at 12:47:27PM -0700, Lukas K. wrote:
That fixed it for me, so feel free to merge.
@ethomson: are you okay with this fix? If so, then I'll see
whether I can create a v0.28.2 release candidate next week
including this and other bugfixes.
|
We assume that if we are on Win32, Amiga OS, Solaris or SunOS,
that the regcomp(3P) function cannot be provided by the system.
Thus we will in these cases always include our own, bundled regex
sources to make a regcomp implementation available. This test is
obviously very fragile, and we have seen it fail on MSYS2/MinGW
systems, which do in fact provide the regcomp symbol. The effect
is that during compilation, we will use the "regex.h" header
provided by MinGW, but use symbols provided by ourselves. This
in fact may cause subtle memory layout issues, as the structure
made available via MinGW doesn't match what our bundled code
expects.
There's one more problem with our regex detection: on the listed
platforms, we will incorrectly include the bundled regex code
even in case where the system provides regcomp_l(3), but it will
never be used for anything.
Fix the issue by improving our regcomp detection code. Instead of
relying on a fragile listing of platforms, we can just use
CHECK_FUNCTION_EXISTSinstead. This will not in fact avoid theheader-ordering problem. But we can assume that as soon as a
system-provided "regex.h" header is provided, that
CHECK_FUNCTION_EXISTSwill now correctly find the desiredsymbol and thus not include our bundled regex code.
Please do not merge this, it's got maint/v0.28 as target. Thing is on master, we'll get this fixed via @ethomson's #4935. But we surely do not want to backport that PR to v0.28.2, so I think we should have a simple fix available for maint.