GNOME Bugzilla – Bug 720504
gir: duplicate definition of UNLOCK_OPTION_IDLE
Last modified: 2016-02-25 17:24:11 UTC
The constant UNLOCK_OPTION_IDLE appears to be making it into both Gcr-3.gir and GcrUi-3.gir. That causes the build to fail on vapigen of gcr-ui, like so: GICOMP GcrUi-3.gir VAPIGEN gcr-ui-3.vapi GcrUi-3.gir:1586.5-1588.58: error: `Gcr' already contains a definition for `UNLOCK_OPTION_IDLE' gcr-3.vapi:431.2-431.40: note: previous definition of `UNLOCK_OPTION_IDLE' was here It looks like the appearance of this constant in GcrUi-3.gir is incorrect, because it is in the GCR namespace and doesn't appear anywhere in ui/'s headers.
This is definitely an issue in the gobject-introspection scanner. The logic that decides if we record a symbol that we see looks like this: void gi_source_scanner_add_symbol (GISourceScanner *scanner, GISourceSymbol *symbol) { ... if (scanner->macro_scan || g_hash_table_contains (scanner->files, scanner->current_file)) scanner->symbols = g_slist_prepend (scanner->symbols, gi_source_symbol_ref (symbol)); ie: only add symbols for .h files that were directly given on the commandline. Problem is, when fed this input (from cpp): [snip] # 1 "/home/desrt/.cache/jhbuild/checkout/gcr/gcr/gcr-unlock-options.h" 1 [snip] #define __GCR_UNLOCK_OPTIONS_H__ #define GCR_UNLOCK_OPTION_ALWAYS "always" #define GCR_UNLOCK_OPTION_SESSION "session" #define GCR_UNLOCK_OPTION_TIMEOUT "timeout" #define GCR_UNLOCK_OPTION_IDLE "idle" # 32 "/home/desrt/.cache/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h" 2 #define GCR_TYPE_UNLOCK_OPTIONS_WIDGET (gcr_unlock_options_widget_get_type ()) #define GCR_UNLOCK_OPTIONS_WIDGET(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), GCR_TYPE_UNLOCK_OPTIONS_WIDGET, GcrUnlockOptionsWidget)) [snip] 'scanner->current_file' gets updated to "/home/desrt/.cache/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h" before GCR_UNLOCK_OPTION_IDLE is fed to gi_source_scanner_add_symbol, so it appears as though the definition is coming from the other file.
I think the problem comes from the fact that the process_linemarks() is in the lexer and gi_source_scanner_add_symbol() is called from the parser -- the lexer is just one step ahead of the parser...
Created attachment 264261 [details] [review] gcr: work around issue in giscanner
Review of attachment 264261 [details] [review]: This is Stef's code, but I see no reason not to commit this now; even if g-ir-scanner is fixed now, people will still build gcr with older versions.
Comment on attachment 264261 [details] [review] gcr: work around issue in giscanner Attachment 264261 [details] pushed as 444c8c5 - gcr: work around issue in giscanner Leaving open to solve the bug (which as far as I can tell is not yet fixed).
This breaks the Gcr build: /data/src/gcr/gcr/gcr-unlock-options.h:30: syntax error, unexpected ';' in '/* Delete this line when https://bugzilla.gnome.org/show_bug.cgi?id=720504 is fixed */;' at ';'
This worked at least with the gcc that's in Fedora 20. What compiler emitted that error? I tested putting only a comment there but the lexer just skipped right over it and the bug remained, so we need something more. That said, I'm OK with putting anything at all there as long as it causes the lexer to slow down a step...
*** Bug 726008 has been marked as a duplicate of this bug. ***
(In reply to comment #1) > 'scanner->current_file' gets updated to > "/home/desrt/.cache/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h" before > GCR_UNLOCK_OPTION_IDLE is fed to gi_source_scanner_add_symbol, so it appears as > though the definition is coming from the other file. Ryan, are you sure about this? I have the same error, but I'm not seeing that behavior. I sprinkled a little debugging and I see this: process_linemarks: current_file <- /home/mcatanzaro/jhbuild/checkout/gcr/gcr/gcr-unlock-options.h /home/mcatanzaro/jhbuild/checkout/gcr/gcr/gcr-unlock-options.h:19: syntax error, unexpected OBJECT_MACRO in '#define __GCR_UNLOCK_OPTIONS_H__ ' at '#define __GCR_UNLOCK_OPTIONS_H__' yyparse: Adding symbol: #define GCR_UNLOCK_OPTION_SESSION yyparse: Adding symbol: #define GCR_UNLOCK_OPTION_TIMEOUT yyparse: Adding symbol: #define GCR_UNLOCK_OPTION_IDLE process_linemarks: current_file <- /home/mcatanzaro/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h yyparse: Adding symbol: #define GCR_TYPE_UNLOCK_OPTIONS_WIDGET /home/mcatanzaro/jhbuild/checkout/gcr/ui/gcr-unlock-options-widget.h:34: syntax error, unexpected '(' in '#define GCR_UNLOCK_OPTIONS_WIDGET(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), GCR_TYPE_UNLOCK_OPTIONS_WIDGET, GcrUnlockOptionsWidget))' at '(' Looks like it's being processed properly to me. I will attach my debug patch in the off chance I missed printing debug for an assignment to current_file (I don't think I did). Note that last "syntax error" reported by bison: that is wrong and relevant. Now, in bug #726008 I reported that I can only reproduce this when CFLAGS is set in my jhbuildrc. I bisected the problem to this commit: cbafcdb323307fd6bb5e48b44167fd995dc933a1 is the first bad commit commit cbafcdb323307fd6bb5e48b44167fd995dc933a1 Author: Ryan Lortie <desrt@desrt.ca> Date: Sun Dec 8 11:48:08 2013 -0500 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. https://bugzilla.gnome.org/show_bug.cgi?id=720063 And that bisection was surely correct. In bug #726008 I reported that if I have the following line in my jhbuildrc: os.environ['CFLAGS'] = '' Then I wind up with GCR_UNLOCK_OPTION_IDLE in both gir files, and the build breaks. That was a year ago; I cannot reproduce that anymore, and I'm pretty sure I was wrong/careless in restarting my jhbuild environment between testings and that the empty CFLAGS doesn't actually harm anything. If I had been more careful a year ago, we would have figured this out much sooner, because I would have tried some more combinations of CFLAGS to see what was up. I *can* still break the build by passing this: os.environ['CFLAGS'] = '-ggdb3' But this works fine: os.environ['CFLAGS'] = '-g' I tested this several times to make sure. It makes lots more sense. Back to your first example: you are looking at output from cpp, so why are there #defines left if it has already been through cpp? By default, there will never be #defines in the preprocessed output. But with debugging turned up high enough, there will be #defines in the output so that gcc can use the original constants when it generates DWARF. I bet that's what's tripping up the scanner. And it explains why only a few of us get this bug. This explanation is also consistent with the terrible error spew of unprocessed macros from gi-scanner introduced around that time (the scanner is tripping up on all the new #defines). gi-scanner doesn't know how to handle most #defines: they are completely outside of its grammar. When bison hits such a macro, yyerror() gets called, and gi-scanner suppresses the output if it's not doing a "macro scan" -- a separate step where it processes the macros from all the passed files (since it can't normally do that after they go through cpp). But if those #defines show up in the cpp output, then it does not suppress the error message from bison. Note: I can conceivably see some other weird effect if your CFLAGS were different than mine, such that your CFLAGS really did cause the reported file to change too soon, but that seems unlikely and I can't think of what this might be. If so, then filtering out particular flags would be less desirable. Probably you were just having a bad debug day when you reported this bug? So, as for a solution, I see three options: * Pass only CPPFLAGS to cpp and not CFLAGS. I think this is wrong, because I think autotools passes both, correct? * Teach the scanner to skip over #defines when it's processing something that's already been through cpp. Somebody else can implement this. :) * Just filter out -ggdb3 and related flags and move on with our lives. Let's also remember to remove the weird workaround (which doesn't work for me at all) from gcr. Lastly, I want to see what breaks if we make syntax errors fatal when not doing a macro scan. If it doesn't break anything (or doesn't break much) then let's do that.
Created attachment 295710 [details] [review] debug patch
Created attachment 295711 [details] [review] Remove workaround for bug #720504
Created attachment 295712 [details] [review] scanner: make parse errors fatal If we hit something outside our grammar when not doing a macro scan, that's probably a bad problem. Treat it as fatal. This patch might be a bad idea; I'm going to try building meta-gnome-core and meta-gnome-apps-tested with it to see if anything breaks. Or we could throw it up on Continuous and see what happens....
Created attachment 295713 [details] [review] scanner: don't pass certain debug level flags to cpp These may cause cpp to output code that still has #defines in them, which the scanner does not expect. Not sure what I think about this, but it fixes the bug.
(In reply to comment #12) > This patch might be a bad idea; I'm going to try building meta-gnome-core and > meta-gnome-apps-tested with it to see if anything breaks. Or we could throw it > up on Continuous and see what happens.... I made it as far as cogl (40/242): GISCAN Cogl-1.0.gir /home/mcatanzaro/jhbuild/checkout/cogl/cogl/deprecated/cogl-texture-deprecated.h:45: syntax error, unexpected '*', expecting ')' or ',' in 'cogl_texture_get_format (CoglTexture *texture);' at '*' /home/mcatanzaro/jhbuild/install/share/gobject-introspection-1.0/Makefile.introspection:153: recipe for target 'Cogl-1.0.gir' failed which is pretty straightforward: CoglPixelFormat cogl_texture_get_format (CoglTexture *texture); Oh well.
(In reply to comment #9) > This explanation is also consistent with the terrible error spew of unprocessed > macros from gi-scanner introduced around that time (the scanner is tripping up > on all the new #defines). gi-scanner doesn't know how to handle most #defines: > they are completely outside of its grammar. Logic checks out until we get here. If #define is totally outside of the scope of understanding of the scanner then how do we end up with the #define'd constants making it into the .gir? How were we getting only some defines through earlier, but not others?
(In reply to comment #15) > Logic checks out until we get here. If #define is totally outside of the scope > of understanding of the scanner then how do we end up with the #define'd > constants making it into the .gir? Not totally, but "mostly." The lexer has this: "#define "[a-zA-Z_][a-zA-Z_0-9]*"(" { yyless (yyleng - 1); return FUNCTION_MACRO; } "#define "[a-zA-Z_][a-zA-Z_0-9]* { return OBJECT_MACRO; } And the grammar has this: function_macro : FUNCTION_MACRO <snip>; object_macro : OBJECT_MACRO <snip>; function_macro_define : function_macro '(' identifier_list ')' ; object_macro_define : object_macro constant_expression <snip>; // this <snip> is where defined constants get added to the gir That syntax error occurs when an object_macro nonterminal is not followed by a constant_expression nonterminal. That triggers a call to yyerror(), where the scanner simply ignores the error, and prints it only if not doing a macro scan. The error is actually part of the grammar: macro : function_macro_define | object_macro_define | preproc_conditional | error ; So the "error" is treated as a nonterminal that expands to a macro, and bison moves on from there. > How were we getting only some defines through earlier, but not others? I dunno how cpp decides which defines to let through. I also didn't debug this far enough to say how exactly the defines are causing the problem, but they surely are.
Created attachment 295767 [details] [review] scanner: add symbols for macros only during a macro scan Otherwise, using debugging flags like -ggdb3 will cause breakage, as macros will show up unexpectedly in the output of cpp. I like this approach better than removing the flags, so I will obsolete attachment #295713 [details]. This fixes all build breakage, but unlike attachment #295713 [details] it does NOT fix the horrible error spew. I will investigate a smart way to surpress that.
(In reply to comment #17) > I will investigate a smart way to surpress that. I couldn't find any easy way to do this, so I will un-obsolete attachment #295713 [details].
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
Review of attachment 295713 [details] [review]: ::: giscanner/sourcescanner.py @@ +300,3 @@ + try: + cpp_args.remove(flag) + except: I'm not a fan of "bare except" clauses. Can you change this to only catch KeyError? Fine to commit with that change.
(In reply to Colin Walters from comment #20) > Review of attachment 295713 [details] [review] [review]: > > ::: giscanner/sourcescanner.py > @@ +300,3 @@ > + try: > + cpp_args.remove(flag) > + except: > > I'm not a fan of "bare except" clauses. Can you change this to only catch > KeyError? Fine to commit with that change. OK. I also tried dropping the symbols in the lexer, but couldn't get it to work properly, so dropping the flags manually is the best approach for now.
Comment on attachment 295767 [details] [review] scanner: add symbols for macros only during a macro scan Obsoleting this since it doesn't resolve the error spew.
Comment on attachment 295711 [details] [review] Remove workaround for bug #720504 Attachment 295711 [details] pushed as 21aa2de - Remove workaround for bug #720504
Pushed catching just ValueError (not KeyError) Attachment 295713 [details] pushed as d2dce55 - scanner: don't pass certain debug level flags to cpp