Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#55577 closed enhancement (wontfix)

getting rid of unforeseen -I${prefix}/include option adding and its potential side-effects

Reported by: RJVB (René Bertin) Owned by:
Priority: Normal Milestone:
Component: ports Version:
Keywords: Cc: ryandesign (Ryan Schmidt), larryv (Lawrence Velázquez)
Port: pcre, glib2

Description

I ran into header file conflicts the other day building code that depends indirectly on glib-2. This was on Linux using my MacPorts-for-Linux adaptation but the same thing could easily happen on Mac too because the same rules and guidelines apply for header search path priorities.

The culprit was an unhappy -I/opt/local/include compiler option which caused a build to find headers from $prefix that were incompatible with the ones included and needed by the project.

I traced that option first to glib-2.0.pc which is patched by port:glib2 to add the include path, and then to port:pcre which installs its headers into ${prefix}/include and thus actually needs it.

The proper fix here is:

  1. Install the pcre (and pcre2) headers in a subdirectory with the official mechanism, i.e. add this to configure.args in port file pcre:
                        --includedir=${prefix}/include/${subport} \
    

This change will be reflected in the pcre/pcre2 pkg-config files.

  1. update port:glib2 . This should be automatic but I didn't want to second-guess the reason NOT to use pkgconfig in port configure glib2 which means a few changes are required. No hacks though, if anything the modified version is less of a hack than the original. PCRE_CFLAGS and PCRE_LIBS are now set by querying pkg-config instead of with hardcoded values (idem for the ZLIB variables, as a bonus). Assuming the change to glib-2.0.pc is still required, this can actually be done in such a way that the build system inserts the proper option(s) by adding @PCRE_CFLAGS@ instead of the hardcoded -I option.

After applying the changes above I get the pkg-config output below, which is far more specific

> pkg-config --cflags glib-2.0
-I/opt/local/include/glib-2.0 -I/opt/local/lib/glib-2.0/include -I/opt/local/include/pcre 

Attachments (1)

glib2.diff (2.3 KB) - added by RJVB (René Bertin) 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 in reply to:  description ; Changed 3 years ago by ryandesign (Ryan Schmidt)

Replying to RJVB:

I ran into header file conflicts the other day building code that depends indirectly on glib-2. This was on Linux using my MacPorts-for-Linux adaptation but the same thing could easily happen on Mac too because the same rules and guidelines apply for header search path priorities.

The culprit was an unhappy -I/opt/local/include compiler option which caused a build to find headers from $prefix that were incompatible with the ones included and needed by the project.

Sounds like a bug in that project. It should place the appropriate -I flags for including its own headers before the -I flags from pkg-config and elsewhere that include the system headers.

Switching MacPorts base to using -isystem instead of -I would be another possible solution to this problem which is proposed in #40656.

I traced that option first to glib-2.0.pc which is patched by port:glib2 to add the include path, and then to port:pcre which installs its headers into ${prefix}/include and thus actually needs it.

The proper fix here is:

  1. Install the pcre (and pcre2) headers in a subdirectory with the official mechanism, i.e. add this to configure.args in port file pcre:
                        --includedir=${prefix}/include/${subport} \
    

This change will be reflected in the pcre/pcre2 pkg-config files.

This has the potential to break ports that use pcre/pcre2. Probably not all of them use pkg-config.

  1. update port:glib2 . This should be automatic but I didn't want to second-guess the reason NOT to use pkgconfig in port configure glib2 which means a few changes are required. No hacks though, if anything the modified version is less of a hack than the original. PCRE_CFLAGS and PCRE_LIBS are now set by querying pkg-config instead of with hardcoded values (idem for the ZLIB variables, as a bonus). Assuming the change to glib-2.0.pc is still required, this can actually be done in such a way that the build system inserts the proper option(s) by adding @PCRE_CFLAGS@ instead of the hardcoded -I option.

I'm tentatively ok with changing glib2/glib2-devel back to using pkg-config. It was changed to not use pkg-config back when pkg-config was changed to require glib2, since that would have been a circular dependency which is not allowed in MacPorts. Since then, pkg-config has changed to using a bundled copy of glib. I agree that the current glib2/glib2-devel portfiles and the configure script patches are quite hacky due to the effort taken to avoid pkg-config.

comment:2 in reply to:  1 ; Changed 3 years ago by RJVB (René Bertin)

Replying to ryandesign:

Sounds like a bug in that project. It should place the appropriate -I flags for including its own headers before the -I flags from pkg-config and elsewhere that include the system headers.

That's what I thought first too, but after checking with them (Qt) it turned out the issue was in my build environment. Even if -I options work that way it's not very reliable because when projects get more complex you do not always keep full control over the order in which you're assembling them, nor over which replies from pkg-config contain -I flags that should go all at the end. (FWIW, the conflict arose deep in the QtWebEngine build tree.) That's also why we have such problems avoiding interference from /usr/local/include.

Switching MacPorts base to using -isystem instead of -I would be another possible solution to this problem which is proposed in #40656.

base is not to blame here (beyond possibly the fact it sets CPATH).

This change will be reflected in the pcre/pcre2 pkg-config files.

This has the potential to break ports that use pcre/pcre2. Probably not all of them use pkg-config.

That would be a bug in those ports, easily fixable (if they don't use pkg-config they already must be told where to find the header directory). And easy to check for in MacPorts (revbump all dependents).

I'm tentatively ok with changing glib2/glib2-devel back to using pkg-config.

I haven't tried and so cannot guarantee that this will indeed be sufficient to take glib2 out of the equation here. All I know is that pkg-config --cflags glib-2.0 includes the result for pkg-config --cflags pcre.

I didn't get around to attaching the glib2 patch, will do so now.

Changed 3 years ago by RJVB (René Bertin)

Attachment: glib2.diff added

comment:3 in reply to:  2 ; Changed 3 years ago by ryandesign (Ryan Schmidt)

Replying to RJVB:

Replying to ryandesign:

Sounds like a bug in that project. It should place the appropriate -I flags for including its own headers before the -I flags from pkg-config and elsewhere that include the system headers.

That's what I thought first too, but after checking with them (Qt) it turned out the issue was in my build environment. Even if -I options work that way it's not very reliable because when projects get more complex you do not always keep full control over the order in which you're assembling them, nor over which replies from pkg-config contain -I flags that should go all at the end. (FWIW, the conflict arose deep in the QtWebEngine build tree.)

MacPorts policy has been that it is the responsibility of the build system to put the -I flags in the correct order. Sometimes, for complex build systems, it's not easy to fix such problems, but we still maintain that that is the best fix.

That's also why we have such problems avoiding interference from /usr/local/include.

Build systems that add flags for using /usr/local are certainly a problem and they should be told not to do that in MacPorts. But that's not why /usr/local is a problem for MacPorts. Rather, it's that compilers look in /usr/local/include and /usr/local/lib by default, even when there is no corresponding -I or -L flag. See wiki:FAQ#usrlocal.

Switching MacPorts base to using -isystem instead of -I would be another possible solution to this problem which is proposed in #40656.

base is not to blame here (beyond possibly the fact it sets CPATH).

I didn't say base was to blame, I said making that change in base would be a solution to the problem.

Setting CPATH (and LIBRARY_PATH) is not a bug, but a feature.

This change will be reflected in the pcre/pcre2 pkg-config files.

This has the potential to break ports that use pcre/pcre2. Probably not all of them use pkg-config.

That would be a bug in those ports, easily fixable (if they don't use pkg-config they already must be told where to find the header directory). And easy to check for in MacPorts (revbump all dependents).

Not that easy to check for, since someone would need to look through the logs of all the failed builds, and then fix them. Probably not all of the failed builds will be because of pcre. I count 131 ports depending on pcre or pcre2.

I'm tentatively ok with changing glib2/glib2-devel back to using pkg-config.

I haven't tried and so cannot guarantee that this will indeed be sufficient to take glib2 out of the equation here. All I know is that pkg-config --cflags glib-2.0 includes the result for pkg-config --cflags pcre.

I didn't get around to attaching the glib2 patch, will do so now.

That's not particularly the type of patch I was expecting. I was expecting a patch that adds the pkgconfig build dependency, removes the environment variables that tell the configure script not to use pkg-config and specify the custom flags, removes the custom code from the pre-configure block for reading libffi's pkg-config file, and reworks the patchfiles to patch configure.ac instead of configure and runs autogen.sh, since the only reason we weren't running autogen.sh is that that would require pkg-config.

I should point out though that the reason why -I${prefix}/include is in the .pc file is not because of pcre but because of libintl; see #9937. The developers of Qt have said that we should not be doing that, and I filed #44395 about undoing it, but looking at it again now, the Qt developer did not give a justification that I find satisfactory, and I'm not sure what other solution we could use.

comment:4 in reply to:  3 ; Changed 3 years ago by RJVB (René Bertin)

Replying to ryandesign:

MacPorts policy has been that it is the responsibility of the build system to put the -I flags in the correct order. Sometimes, for complex build systems, it's not easy to fix such problems, but we still maintain that that is the best fix.

I must not have been clear. This approach would ultimately oblige port maintainers to plow through the build system trying to figure out how to split the sources of the -I flags and then patch the system so that those flags are placed in what MacPorts thinks is the correct order. Nobody is perfect, but if builds succeed everywhere except in a certain build/packaging/distribution system, guess where the blame will be put.

I maintain that the proper fix is to avoid the problem as far as possible.

Note how big and complex build systems (think Qt and KF5) now generate enormous compiler commands because every (sub)component puts its headerfiles in dedicated subdirectories that are then listed explicitly with as many -I flags, while the code is supposed just to do #include <foo.h>.

Also keep in mind that many complex projects are designed with the idea in mind that they should be built in a clean environment, like the build bots but also their CI systems do. That affects mostly issues that arise when building a new version with the old version already installed in the target location, but it's not too much of a stretch for them to expect users to remove at least the dev files of conflicting packages.

Rather, it's that compilers look in /usr/local/include and /usr/local/lib by default, even when there is no corresponding -I or -L flag.

I know, and that's exactly what I meant. For /usr/local/include this cannot be avoided, but it can for ${prefix}/include .

I didn't say base was to blame, I said making that change in base would be a solution to the problem.

Again, only as far as it's caused by base. I doubt that the pcre build system is clever enough NOT to add ${prefix}/include to its required cflags when base starts adding that path via -isystem.

Setting CPATH (and LIBRARY_PATH) is not a bug, but a feature.

DIdn't say it was, don't know how necessary it is (to set CPATH) either, and this is easy to undo in the Portfiles that require it.

Not that easy to check for, since someone would need to look through the logs of all the failed builds, and then fix them. Probably not all of the failed builds will be because of pcre. I count 131 ports depending on pcre or pcre2.

You can rather safely exclude all those that use CMake, and for the remainder do a quick grep -i 'pkg.*pcre' -R ${worksrcpath} --include="*.ac" to exclude the ones that clearly do use pkg-config to find the dependency. Of course one can install a few symlinks to expose the headerfiles in ${prefix}/include if you really want to be sure no ports will break. But I'd say this should break as much as a regular upgrade of the dependency would, and could be treated as such.

In the end it will be the pcre maintainer who decides - or the pcre developers could agree that nowadays it's better to put their headers in their own directory O:-)

That's not particularly the type of patch I was expecting.

Evidently it's the patch that I tested, which changes as little as possible.

Why would it be necessary to patch configure.ac?

I should point out though that the reason why -I${prefix}/include is in the .pc file is not because of pcre but because of libintl; see #9937.

That is no longer unambiguously clear from the current comments above the patches. Is it still true?

The developers of Qt have said that we should not be doing that,

I got very similar reactions ("the package is broken. .pc files must not ever list system paths.") to my query on their ML. I agree with that guideline, and thus plan to take this point up with the pcre developers.

and I filed #44395 about undoing it, but looking at it again now, the Qt developer did not give a justification that I find satisfactory, and I'm not sure what other solution we could use.

First thing would be to check if a solution is still required. I've rebuilt a few ports with the patched glib-2.0.pc file without problem, but not enough yet to be confident that there's no longer a reason to add a specific -I for libintl.h

Another solution to use would be to put libintl.h and family in their own directory too.

Then there's the easy solution: patch the #include statement of the file(s) that can't find libintl.h . Maybe use an absolute include path (#include </opt/local/include/libintl.h>); I don't think I ever tried doing that but if it works it should have less potential side-effects than the .pc file tweak. Or do ln -s ../libintl.h ${prefix}/include/glib-2.0/glib-libintl.h and include that file instead.

comment:5 Changed 3 years ago by RJVB (René Bertin)

Turns out port:git is among the not-so-well-behaved ports that fail to use pkg-config because they skip the entire configure step. Git itself isn't very nicely behaved because it assumes that you only need to specify the pcre lib location.

This fixes the port for any future change in port:pcre:

if {[variant_isset pcre]} {
    depends_build-append \
                    port:pkgconfig
    if {[file exists ${prefix}/bin/pkg-config]} {
        # fetch the flags required for building with the local pcre2 install
        set CFLAGS  "${CFLAGS} [exec ${prefix}/bin/pkg-config --cflags libpcre2-8]"
    }
}
Last edited 3 years ago by RJVB (René Bertin) (previous) (diff)

comment:6 in reply to:  4 Changed 3 years ago by ryandesign (Ryan Schmidt)

Replying to RJVB:

Replying to ryandesign:

MacPorts policy has been that it is the responsibility of the build system to put the -I flags in the correct order. Sometimes, for complex build systems, it's not easy to fix such problems, but we still maintain that that is the best fix.

I must not have been clear. This approach would ultimately oblige port maintainers to plow through the build system trying to figure out how to split the sources of the -I flags and then patch the system so that those flags are placed in what MacPorts thinks is the correct order. Nobody is perfect, but if builds succeed everywhere except in a certain build/packaging/distribution system, guess where the blame will be put.

It doesn't oblige port maintainers to do anything other than report problems to the developers of the software. It's up to the developers of the software to fix their broken build systems. If developers do not comply, then port maintainers can be nice and try to fix it themselves, but they don't have to go to that trouble. They can allow the port to be broken in these situations, and refer users who encounter the problem to the developers; perhaps that motivates the developers to finally fix it.

You presuppose that this problem is unique to the Mac or unique to MacPorts. I claim it is not.

I maintain that the proper fix is to avoid the problem as far as possible.

Which is why I pointed out my longstanding suggestion that base should use -isystem instead of -I.

Also keep in mind that many complex projects are designed with the idea in mind that they should be built in a clean environment, like the build bots but also their CI systems do. That affects mostly issues that arise when building a new version with the old version already installed in the target location, but it's not too much of a stretch for them to expect users to remove at least the dev files of conflicting packages.

If a developer requires that, then that port might well fail to build in MacPorts, unless trace mode is used.

I didn't say base was to blame, I said making that change in base would be a solution to the problem.

Again, only as far as it's caused by base. I doubt that the pcre build system is clever enough NOT to add ${prefix}/include to its required cflags when base starts adding that path via -isystem.

That is not a requirement for my proposal to succeed. If -isystem/opt/local/include is added to the flags, then /opt/local/include will be considered a system location, and will be checked after project locations, even if -I/opt/local/include also appears. So, for a concrete example, if the compile line includes in order the flags -isystem/opt/local/include -I/opt/local/include -Isome/local/path, then the paths checked, in order, will be some/local/path and then /opt/local/include.

That's not particularly the type of patch I was expecting.

Evidently it's the patch that I tested, which changes as little as possible.

Why would it be necessary to patch configure.ac?

glib2 currently patches configure to enable universal building, because patching configure.ac would involve running autoreconf which would require pkgconfig which we were trying to avoid. If we're no longer trying to avoid that, then we would patch configure.ac instead of configure.

I should point out though that the reason why -I${prefix}/include is in the .pc file is not because of pcre but because of libintl; see #9937.

That is no longer unambiguously clear from the current comments above the patches. Is it still true?

Yes, /opt/local/include/glib-2.0/glib/gi18n.h and /opt/local/include/glib-2.0/glib/gi18n-lib.h still #include <libintl.h>.

Another solution to use would be to put libintl.h and family in their own directory too.

I don't see why we would do that. If you think gettext should do that, then I suppose you think all other ports that currently put things directly in /opt/local/include should do that, and I don't think we're going to make all those changes, and the consequent necessary changes in all the ports that use those ports, all to accommodate broken build systems. Fix the broken build systems instead.

Then there's the easy solution: patch the #include statement of the file(s) that can't find libintl.h . Maybe use an absolute include path (#include </opt/local/include/libintl.h>); I don't think I ever tried doing that but if it works it should have less potential side-effects than the .pc file tweak. Or do ln -s ../libintl.h ${prefix}/include/glib-2.0/glib-libintl.h and include that file instead.

You're right that an absolute path in the #include statement should work. I hadn't thought about that, so let me think about it and try it out and see if there are any adverse consequences of making that change.

comment:7 in reply to:  5 Changed 3 years ago by ryandesign (Ryan Schmidt)

Replying to RJVB:

This fixes the port for any future change in port:pcre:

if {[variant_isset pcre]} {
    depends_build-append \
                    port:pkgconfig
    if {[file exists ${prefix}/bin/pkg-config]} {
        # fetch the flags required for building with the local pcre2 install
        set CFLAGS  "${CFLAGS} [exec ${prefix}/bin/pkg-config --cflags libpcre2-8]"
    }
}

This will not work if pkgconfig is not installed at the time the user requests to install git. In general ports should not alter their behavior dependent on whether other MacPorts-installed files are already present. If we were going to add such code, you would do so in a pre-build block, so that it is guaranteed that pkgconfig is installed by then. The way that the git port sets build.args would also have to be altered, to do so at pre-build time. But I wouldn't make these changes to the git port unless we change the pcre port in the way that you've requested.

comment:8 Changed 3 years ago by RJVB (René Bertin)

I cannot really comment on the idea of using -isystem . It might work as you suggest, but it might depend on whether clang or gcc is used. IIRC they differ in the way they do this sort of thing for libraries, they could also for headers. I forget the details and have more urgent things to pursue right now :-/

I don't claim this is specific to MacPorts. I do suppose that any comparable distribution system will run into similar issues sooner or later. One could check if any other such system has solutions in place (Gentoo Prefix would be a good candidate). MacPorts may in fact be a little less vulnerable because the real system header file locations are less likely to contain conflicting older versions.

This will not work if pkgconfig is not installed at the time the user requests to install git.

I guess I should set it in the pre-build indeed? I'm so used to doing the configure and build steps separately that I tend to forget this. I did try to set this up with options variables and defaults, but that became very complicated with a number of unwanted curly braces in the CFLAGS string.

comment:9 Changed 3 years ago by RJVB (René Bertin)

In fact, how does the resolution of build dependencies work?

  • If the (pkgconfig) install is triggered from within the evaluation of the dependent port (say before the pre-configure step) the CFLAGS definition will indeed not be altered after the install
  • If the install is somehow done before the dependent port is parsed (or if the Portfile is parsed again after that install) the check above should pass

comment:10 in reply to:  9 ; Changed 3 years ago by ryandesign (Ryan Schmidt)

Replying to RJVB:

I cannot really comment on the idea of using -isystem . It might work as you suggest, but it might depend on whether clang or gcc is used.

All the compilers we use support -isystem. gcc-4.0 didn't support it properly, but we don't use that anymore.

Replying to RJVB:

In fact, how does the resolution of build dependencies work?

  • If the (pkgconfig) install is triggered from within the evaluation of the dependent port (say before the pre-configure step) the CFLAGS definition will indeed not be altered after the install
  • If the install is somehow done before the dependent port is parsed (or if the Portfile is parsed again after that install) the check above should pass

The build dependencies will be installed by the time the configure phase starts. They might not be there in earlier phases, nor at the time the portfile is parsed.

comment:11 in reply to:  10 Changed 3 years ago by RJVB (René Bertin)

unless we change the pcre port in the way that you've requested.

Suggested would be a better term :)

All the compilers we use support -isystem. gcc-4.0 didn't support it properly, but we don't use that anymore.

I meant differences in how the arguments to the option are used, more specifically the exact resulting order of the search path. I'm not saying there are any just that there might be.

The build dependencies will be installed by the time the configure phase starts. They might not be there in earlier phases, nor at the time the portfile is parsed.

Yes, that much was clear. Somehow I'd expect that "base" would re-parse a portfile after installing any missing dependencies, i.e. that there would be no difference between a (hypothetical)

> port build-dep foo
> port configure foo
> port build foo

and a direct port build foo, regardless of what the Portfile does (or almost).

comment:12 Changed 3 years ago by ryandesign (Ryan Schmidt)

Resolution: wontfix
Status: newclosed

I think the only issue here that we'll make any change for is #44395.

comment:13 Changed 3 years ago by RJVB (René Bertin)

Where would you make the change to using -isystem in the default CPPFLAGS?

comment:14 Changed 3 years ago by ryandesign (Ryan Schmidt)

See #40656 and r115978.

comment:15 Changed 3 years ago by ryandesign (Ryan Schmidt)

In a8ab1a53805b8a0cb80e34639c62266ee4a03b22/macports-ports:

glib2-devel: Update to 2.56.0

See: #56081

Also, patch configure.ac, not configure. The reason why we were patching
configure was that patching configure.ac would have required running
autoreconf, and that would have required pkg-config, and at the time,
pkg-config depended on glib2, and MacPorts cannot accommodate circular
dependencies. pkg-config has included its own private copy of glib2
since f0e3b8c11602b61d2ac6d9ec1f69541c1d197257, so this reason no
longer applies.

We have been maintaining a large configure patch which needs to be
tediously regenerated from the configure.ac patch whenever glib2 is
updated. That effort is saved by being able to use the configure.ac
patch directly.

The Portfile is also simplified by allowing pkg-config to find the
libraries of the dependencies, rather than having to specify each of
them manually.

Reverts 847ba0283b0e043211bcaa4f2f6824f5f290dfa5.

See: #55577

Also, add SIZEOF_SSIZE_T handling to config.h.ed. This should have been
done in 2eb6602f8fbd933b0337bcf89bbcdcb233e8148a when similar patching
was added to the configure script. Add a comment to the patch to remind
the maintainer to update config.h.ed too.

Also, no longer undefine AC_APPLE_UNIVERSAL_BUILD in config.h. It's
unclear why we were ever doing this. config.h is only used at build
time. The only thing AC_APPLE_UNIVERSAL_BUILD currently does is ensure
WORDS_BIGENDIAN is defined correctly. glib doesn't use WORDS_BIGENDIAN;
it uses its own G_BYTE_ORDER, G_BIG_ENDIAN and G_LITTLE_ENDIAN. Maybe
AC_APPLE_UNIVERSAL_BUILD used to do something more objectionable.

comment:16 Changed 3 years ago by ryandesign (Ryan Schmidt)

In db9f7311c52cf7ad198a8aff1fd9c80e8f11d8b5/macports-ports:

glib2: Update to 2.56.0

Closes: #56081

Also, patch configure.ac, not configure. The reason why we were patching
configure was that patching configure.ac would have required running
autoreconf, and that would have required pkg-config, and at the time,
pkg-config depended on glib2, and MacPorts cannot accommodate circular
dependencies. pkg-config has included its own private copy of glib2
since f0e3b8c11602b61d2ac6d9ec1f69541c1d197257, so this reason no
longer applies.

We have been maintaining a large configure patch which needs to be
tediously regenerated from the configure.ac patch whenever glib2 is
updated. That effort is saved by being able to use the configure.ac
patch directly.

The Portfile is also simplified by allowing pkg-config to find the
libraries of the dependencies, rather than having to specify each of
them manually.

Reverts 847ba0283b0e043211bcaa4f2f6824f5f290dfa5.

See: #55577

Also, add SIZEOF_SSIZE_T handling to config.h.ed. This should have been
done in 2eb6602f8fbd933b0337bcf89bbcdcb233e8148a when similar patching
was added to the configure script. Add a comment to the patch to remind
the maintainer to update config.h.ed too.

Also, no longer undefine AC_APPLE_UNIVERSAL_BUILD in config.h. It's
unclear why we were ever doing this. config.h is only used at build
time. The only thing AC_APPLE_UNIVERSAL_BUILD currently does is ensure
WORDS_BIGENDIAN is defined correctly. glib doesn't use WORDS_BIGENDIAN;
it uses its own G_BYTE_ORDER, G_BIG_ENDIAN and G_LITTLE_ENDIAN. Maybe
AC_APPLE_UNIVERSAL_BUILD used to do something more objectionable.

Note: See TracTickets for help on using tickets.