Opened 6 years ago

Last modified 5 years ago

#40656 reopened enhancement

Use -isystem instead of -I in default configure.cppflags

Reported by: ryandesign (Ryan Schmidt) Owned by: macports-tickets@…
Priority: Normal Milestone: MacPorts Future
Component: base Version: 2.2.0
Keywords: haspatch Cc: larryv (Lawrence Velázquez), michaelld (Michael Dickens), mkae (Marko Käning), drkp (Dan Ports), cooljeanius (Eric Gallager)
Port:

Description

We should consider changing the default configure.cppflags from "-I" to "-isystem". -isystem is similar to -I except that the given directory is checked after all "-I" directories are checked. In this way, using "-isystem" instead of "-I" would fix a whole class of problems where some ports sometimes fail to upgrade when a previous version is installed. Several ports (including freetype, ghostscript, pianobar) already make this change, so making this change in base would allow us to simplify those ports. The attached patch changes "-I" to "-isystem" everywhere I could find it.

Attachments (1)

isystem.diff (3.1 KB) - added by ryandesign (Ryan Schmidt) 6 years ago.
proposed patch

Download all attachments as: .zip

Change History (20)

Changed 6 years ago by ryandesign (Ryan Schmidt)

Attachment: isystem.diff added

proposed patch

comment:1 Changed 6 years ago by ryandesign (Ryan Schmidt)

Type: defectenhancement

comment:2 Changed 6 years ago by ryandesign (Ryan Schmidt)

I've been using this fix locally on Mountain Lion and Mavericks with no problems that I could see.

However I found a mention that -isystem causes problems for C++ code on Tiger; if I can reproduce that, then maybe we have to make this change only on Leopard and later.

comment:3 Changed 6 years ago by larryv (Lawrence Velázquez)

Cc: larryv@… added

Cc Me!

comment:4 Changed 6 years ago by raimue (Rainer Müller)

When building MacPorts itself, we should stick to the usual -I. I don't see any advantage of this change there.

Be aware that using -isystem hides all warnings from the header files as they are treated as if they were system headers. I worry this could mask problems during building and make it harder to identify the cause.

There is also the environment variable CPATH for additional include paths as if they were given using -I on the command line (gcc manual). However, older versions of clang do not honor this setting.

Last edited 6 years ago by ryandesign (Ryan Schmidt) (previous) (diff)

comment:5 Changed 6 years ago by michaelld (Michael Dickens)

Cc: michaelld@… added

Cc Me!

comment:6 Changed 6 years ago by ryandesign (Ryan Schmidt)

Right; MacPorts already sets CPATH.

comment:7 Changed 6 years ago by mkae (Marko Käning)

Cc: mk@… added

Cc Me!

comment:8 Changed 6 years ago by drkp (Dan Ports)

Cc: dports@… added

Cc Me!

comment:9 Changed 6 years ago by cooljeanius (Eric Gallager)

Cc: egall@… added

Cc Me!

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

Replying to ryandesign@…:

However I found a mention that -isystem causes problems for C++ code on Tiger; if I can reproduce that, then maybe we have to make this change only on Leopard and later.

I have indeed seen this problem, e.g. in texlive-bin: #41528. But switching Tiger's default compiler from gcc-4.0 to apple-gcc-4.2 should be the end of that problem: #41782.

Replying to raimue@…:

Be aware that using -isystem hides all warnings from the header files as they are treated as if they were system headers. I worry this could mask problems during building and make it harder to identify the cause.

I didn't know this would hide warnings, but that's an acceptable tradeoff for me. Every week or so, I encounter another ticket about a build problem that I did not see on my system because I was using the patch in this ticket. It fixes so many problems with one simple change that we should definitely make this change.

As for it treating headers as if they were system headers: they are system headers—that is, they are headers installed by the MacPorts system. No need, IMHO, to differentiate that from headers installed by the operating system; they're all just headers provided by software other than the one being compiled. Anytime "#include <foo>" is written, it's telling the preprocessor "please include the system header named 'foo'"; by using -isystem, we're telling the preprocessor (an additional place) where those headers may be found.

There is also the environment variable CPATH for additional include paths as if they were given using -I on the command line (gcc manual). However, older versions of clang do not honor this setting.

Yes, with the difference that paths specified in CPATH are interpreted as if they were given "after any paths given with -I options on the command line" which would indeed be what we want.

MacPorts already sets CPATH and the corresponding LIBRARY_PATH, since several years. I'm not sure if this could completely replace our global -I / -L flags in CPPFLAGS and LDFLAGS. The documentation doesn't show my any reason why it couldn't, but then why haven't we stopped setting -I / -L? A couple years ago lack of support from older clang versions concerned me, but by this point in time we could afford to just blacklist them globally.

One advantage to -isystem is that any path specified with both -I and -isystem is interpreted as if it were specified only with -isystem. This would probably fix a few additional build failures (i.e. from ports that use pkg-config or other -config script which drags -I/opt/local/include back into the CPPFLAGS which could introduce a build failure if the order of the -I flags is wrong).

Last edited 6 years ago by ryandesign (Ryan Schmidt) (previous) (diff)

comment:11 Changed 6 years ago by ryandesign (Ryan Schmidt)

Resolution: fixed
Status: newclosed

comment:12 Changed 6 years ago by jmroot (Joshua Root)

This changes the include order in subtle ways: -I${prefix}/include -I${prefix}/include/some_subdir is different to -isystem${prefix}/include -I${prefix}/include/some_subdir. Definite potential for confusing breakage.

comment:13 Changed 6 years ago by ryandesign (Ryan Schmidt)

I've been using this patch since I opened this ticket 3 months ago with only positive results, as far as I can tell. Of course I have not tested all ports, and the scenario you propose is possible. However I think the number of issues this change will fix will far outnumber the new problems it creates.

comment:14 Changed 6 years ago by raimue (Rainer Müller)

As I said earlier the behavior we want would be the environemnt variable CPATH:

CPATH specifies a list of directories to be searched as if specified with -I, but after any paths given with -I options on the command line.

The only catch is that old clang versions do not support it and I can't remember which (Apple) clang version introduced it. I propose we switch to CPATH when using Xcode >= 5. That version would definitely be new enough and required for both our currently supported platforms. Same for LIBRARY_PATH.

comment:15 in reply to:  14 Changed 6 years ago by ryandesign (Ryan Schmidt)

Replying to raimue@…:

I propose we switch to CPATH when using Xcode >= 5. That version would definitely be new enough and required for both our currently supported platforms. Same for LIBRARY_PATH.

MacPorts already sets CPATH and LIBRARY_PATH in all phases and has done so for years.

comment:16 Changed 5 years ago by jmroot (Joshua Root)

So this breaks some things that parse flags and don’t know about -isystem, right? Specifically swig, which is used kind of a lot.

https://lists.macosforge.org/pipermail/macports-dev/2014-January/025753.html

Can we please revert this and clear or change configure.cppflags in only the ports that need it, or if you don’t care about old clang as per comment:10 just don’t set CPPFLAGS to anything by default, and reply on CPATH?

comment:17 in reply to:  16 ; Changed 5 years ago by ryandesign (Ryan Schmidt)

Replying to jmr@…:

So this breaks some things that parse flags and don’t know about -isystem, right? Specifically swig, which is used kind of a lot.

https://lists.macosforge.org/pipermail/macports-dev/2014-January/025753.html

Can we please revert this and clear or change configure.cppflags in only the ports that need it, or if you don’t care about old clang as per comment:10 just don’t set CPPFLAGS to anything by default, and reply on CPATH?

I see you reverted this in r118016. I would like to reconsider that reversion because I stand by the change.

It fixes a whole host of build failures due to incorrect -I flag ordering that are often not found during the normal course of updating a port, because they may be caused by having an older version of the same port installed, or by having another port installed that coincidentally installs headers whose files have the same names as private headers this port builds with. These problems either go unfixed, or it takes a MacPorts developer significant time and effort to track down the cause and then learn enough about the project's build system to integrate the fix. Our time and effort can be best spent elsewhere, and we can increase the happiness of our users by solving this whole class of problem instantly, centrally, without needing to bother the user with build failures and filing tickets.

I have been using this patch for months without problems. This problem you've mentioned with swig is the first problem I've heard of. Could you be more specific? Could we fix that problem instead?

I do not believe that the suggestion to rely on CPATH entirely solves the problem, because many ports will use pkg-config or other -config scripts, most of which will reintroduce -I/opt/local/include into CPPFLAGS and thus reintroduce the problem. Using -isystem/opt/local/include in CPPFLAGS, as my patch did, supersedes any -I/opt/local/include that may also be specified.

comment:18 Changed 5 years ago by ryandesign (Ryan Schmidt)

Resolution: fixed
Status: closedreopened

comment:19 in reply to:  17 Changed 5 years ago by jmroot (Joshua Root)

Replying to ryandesign@…:

I have been using this patch for months without problems. This problem you've mentioned with swig is the first problem I've heard of. Could you be more specific? Could we fix that problem instead?

As per the link, lal fails to build.

Let’s be clear: it is a bug when a build system passes externally-supplied CPPFLAGS to the compiler before its own internal include paths. IMO we should be working with upstream to fix such bugs, not applying a workaround globally. Even in cases where there is no upstream, just having a patch is more valuable to others in the larger community who encounter the same problem building the software, compared to having an invisible workaround.

But it appears that I’m outvoted, so I’ll go along with reintroducing the change into trunk provided the problem with swig is fixed, and no other problems turn up.

Note: See TracTickets for help on using tickets.