After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 720063 - Makefile.introspection ignores CFLAGS and LDFLAGS when scanning
Makefile.introspection ignores CFLAGS and LDFLAGS when scanning
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gobject-introspection Maintainer(s)
gobject-introspection Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-12-08 15:49 UTC by Allison Karlitskaya (desrt)
Modified: 2015-02-07 17:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
scanner: make sure we pass CFLAGS to cpp (980 bytes, patch)
2013-12-08 16:49 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Honour CPPFLAGS as we do CFLAGS (1.97 KB, patch)
2013-12-22 03:19 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2013-12-08 15:49:14 UTC
I'm trying to get jhbuild working on FreeBSD.  I have the following CFLAGS and LDFLAGS:

CFLAGS=-isystem /usr/local/include
LDFLAGS=-Wl,-Y/usr/local/lib

This is basically the same as -I and -L except that it adds /usr/local/lib in such a way that it is searched last, after all other -I and -L options.  This is important to make sure builds pick up the things in the jhbuild prefix before the things in /usr/local.

FreeBSD installs its packages and ports to /usr/local but the compiler search path does not include it by default for some reason.  Easy enough to fix with the above in my jhbuild environment, though.


Makefile.introspection, when calling the scanner, does this:

...
          $($(_gir_name)_SCANNERFLAGS) \
          --cflags-begin \
          $($(_gir_name)_CFLAGS) \
          --cflags-end \
          $($(_gir_name)_LDFLAGS) \
          $$^ \
...

which doesn't use the user's $(CFLAGS) and $(LDFLAGS) at all

I can modify this to also include $(CFLAGS) and that works nicely (due to the nice --cflags-begin --cflags-end stuff) but if I include my $(LDFLAGS) here as well then I have trouble, since the scanner doesn't recognise -Wl,... options, complaining "g-ir-scanner: error: no such option: -W".

Two possible fixes there:

 - pass -Wl, options through unharmed, just as we do for -I and -L
 - add --ldflags-start and --ldflags-end and use them from the Makefile

...plus adding $(CFLAGS) and $(LDFLAGS) to the Makefile
Comment 1 Colin Walters 2013-12-08 15:52:39 UTC
How does the FreeBSD port system make this work?  Or do they have a g-i build?

(In reply to comment #0)

> FreeBSD installs its packages and ports to /usr/local but the compiler search
> path does not include it by default for some reason.  Easy enough to fix with
> the above in my jhbuild environment, though.

But how does gtk+ find glib with the ports? 

I'm not opposed to adding CFLAGS/LDFLAGS, but a *lot* of components and apps use g-i, and it seems likely to provoke build breakage of some form.  (Although I can't think offhand what that might be)
Comment 2 Allison Karlitskaya (desrt) 2013-12-08 16:43:06 UTC
The reason that this works in ports builds on FreeBSD is pretty accidental.  They use system pcre, which has an include dir of /usr/local/include.  This gets returned by the pkg-config --cflags for glib-2.0, and thus ends up in the per-target CFLAGS and passed through accordingly.  ie: <libintl.h> is only found because pcre happens to be in the same place.

Gtk+ is able to find GLib because GLib's include path is -I/usr/local/include/glib-2.0 and that also comes from pkg-config.  That's the usual behaviour.


After doing a bit more checking, the dumper is fine because it does this:

        cflags = os.environ.get('CFLAGS', '')
        for cflag in cflags.split():
            args.append(cflag)
        ldflags = os.environ.get('LDFLAGS', '')
        for ldflag in ldflags.split():
            args.append(ldflag)

The scanner doesn't do that, though.  Adding CFLAGS there is probably the easiest way to solve this.
Comment 3 Allison Karlitskaya (desrt) 2013-12-08 16:46:26 UTC
It could be argued that it is the fault of GLib for installing a .pc file that doesn't give the full arguments needed for building it correctly (since it is its headers that make use of libintl.h) but it's hard to argue that it should be aware of the user's choice of CFLAGS and attempt to interpret them, particular when they contain exotic things like "-isystem ...".

I think the best expectation here is that if the user consistently uses the same set of CFLAGS/LDFLAGS then they should expect the whole stack to work... and that would be the case here, except that the scanner is running without those CFLAGS.  I think that's why, above anything else, this change is justified.
Comment 4 Allison Karlitskaya (desrt) 2013-12-08 16:49:27 UTC
Created attachment 263746 [details] [review]
scanner: make sure we pass CFLAGS to cpp

When doing the source scanning in giscanner, make sure we pass the
user's CFLAGS environment variable to the compiler, as we do for the
dumper.
Comment 5 Colin Walters 2013-12-08 17:06:24 UTC
Review of attachment 263746 [details] [review]:

The main case I can think of where things might start failing is if builders have e.g. CFLAGS='-Werror=foo' where foo is some warning in the code generated by the scanner.  But, I haven't see any warnings out of the scanner lately, so hopefully this'll be fine.
Comment 6 Allison Karlitskaya (desrt) 2013-12-08 17:24:14 UTC
Strictly speaking, we should also add in CPPFLAGS since it was possible that CPPFLAGS was given when building GLib... In fact, now that I'm on to gdk-pixbuf I notice that some ./configure checks depend on my -isystem being in CPPFLAGS.

CPPFLAGS would be slightly riskier because we don't have them in the dumper already, so we don't know if it could break stuff.  Your call.

Meanwhile, I've set both CFLAGS and CPPFLAGS.
Comment 7 Allison Karlitskaya (desrt) 2013-12-08 17:24:49 UTC
Comment on attachment 263746 [details] [review]
scanner: make sure we pass CFLAGS to cpp

Attachment 263746 [details] pushed as cbafcdb - scanner: make sure we pass CFLAGS to cpp
Comment 8 Colin Walters 2013-12-08 19:12:35 UTC
(In reply to comment #6)
> Strictly speaking, we should also add in CPPFLAGS since it was possible that
> CPPFLAGS was given when building GLib... In fact, now that I'm on to gdk-pixbuf
> I notice that some ./configure checks depend on my -isystem being in CPPFLAGS.
> 
> CPPFLAGS would be slightly riskier because we don't have them in the dumper
> already, so we don't know if it could break stuff. 

I'm ok with adding CPPFLAGS too.
Comment 9 Allison Karlitskaya (desrt) 2013-12-22 03:19:47 UTC
Created attachment 264746 [details] [review]
Honour CPPFLAGS as we do CFLAGS

In all of the places that we pass through the CFLAGS, we should be doing
the same with the CPPFLAGS.
Comment 10 Colin Walters 2013-12-22 22:43:08 UTC
Review of attachment 264746 [details] [review]:

Looks reasonable to me.
Comment 11 Allison Karlitskaya (desrt) 2013-12-23 00:32:51 UTC
Attachment 264746 [details] pushed as ce190a6 - Honour CPPFLAGS as we do CFLAGS
Comment 12 Michael Catanzaro 2014-06-02 03:14:36 UTC
(In reply to comment #1)
> I'm not opposed to adding CFLAGS/LDFLAGS, but a *lot* of components and apps
> use g-i, and it seems likely to provoke build breakage of some form.  (Although
> I can't think offhand what that might be)

This is causing Bug #726008#c10
Comment 13 Michael Catanzaro 2015-01-27 01:33:23 UTC
Ryan, are you OK with reverting this change, since it caused bug #726008 and we don't know how to fix it? I'm getting quite tired of leaving my CFLAGS empty....
Comment 14 Allison Karlitskaya (desrt) 2015-01-28 09:29:26 UTC
No -- I don't think reverting this is correct.  Bug 726008 is really a bug in the gobject-introspection lexer/parser and ought to be fixed there.
Comment 15 André Klapper 2015-02-07 17:00:38 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]