1224452 - CompileDB contains "-Wno-" flags instead of "-Wno-#pragma-messages"

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

I was going to file a separate bug, but this is the really the same issue: the -DPACKAGE_BUGREPORT flags from cairo, graphite2 and ots are wrong, and everything is rooted to the use of shlex.shlex, which is completely broken for this kind of job.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Move shell_quote to the shellutil module

Also make it only quote when actually necessary.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Use mozbuild.shellutil.split instead of shlex.shlex in mozbuild.compilation.util

And then properly quote the strings when printing them out in the compilation
database or the mach compileflags output.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Straighten up a couple things in some moz.builds

We're going to change how e.g. CFLAGS are printed out in backend.mk, and
to fit that model, the data in the corresponding moz.build variables
need to be straightened up.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Better handle make variable references from gyp files

This further improves the changes from bug 1224460 to e.g. handle variable
references mixed with text, and to avoid adding empty strings to the
resulting flags variables when the expansion leads to an empty string.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Quote passthru variables

Ideally, we should properly make and shell quote everything we print out
in makefiles, but that's a can of worms I don't want to open just yet. So
I'll limit myself to just passthru variables.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8691178 [details] [diff] [review]
Move shell_quote to the shellutil module

Review of attachment 8691178 [details] [diff] [review]:
-----------------------------------------------------------------

DIY shell quoting is how CVEs are born. But this code looks fine AFAICT.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8691182 [details] [diff] [review]
Quote passthru variables

Review of attachment 8691182 [details] [diff] [review]:
-----------------------------------------------------------------

I'm kinda surprised we didn't encounter issues with this before!

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
@@ +298,5 @@
>                  'MOZBUILD_CFLAGS += -w',
>              ],
>              'MOZBUILD_CXXFLAGS': [
>                  'MOZBUILD_CXXFLAGS += -fcxx-exceptions',
> +                'MOZBUILD_CXXFLAGS += \'-option with spaces\'',

Perhaps you could change this to a double quoted string to avoid the Python escaping.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8691181 [details] [diff] [review]
Better handle make variable references from gyp files

Review of attachment 8691181 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ +22,5 @@
>      memoize,
>  )
>  from .reader import SandboxValidationError
>  
> +VARIABLES_RE = re.compile('^(.*?)\$\((\w+)\)(.*)$')

Isn't ".*?" the same as ".*"?

.*? -> the character class "." occurring 0 or more times (*) itself 0 or 1 times (?)

Also, the .* shouldn't be necessary: re.split() splits a string based on a regexp. I think that's more appropriate here.

@@ +24,5 @@
>  from .reader import SandboxValidationError
>  
> +VARIABLES_RE = re.compile('^(.*?)\$\((\w+)\)(.*)$')
> +
> +def expand(s, data):

docstring please!

Comment 10

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8691181 [details] [diff] [review]
> Better handle make variable references from gyp files
> 
> Review of attachment 8691181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/frontend/gyp_reader.py
> @@ +22,5 @@
> >      memoize,
> >  )
> >  from .reader import SandboxValidationError
> >  
> > +VARIABLES_RE = re.compile('^(.*?)\$\((\w+)\)(.*)$')
> 
> Isn't ".*?" the same as ".*"?

".*?" is "non-greedy .*"

Comment 11

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #8)
> Comment on attachment 8691182 [details] [diff] [review]
> Quote passthru variables
> 
> Review of attachment 8691182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm kinda surprised we didn't encounter issues with this before!

We haven't encountered it because:
- We had the # quoted in the moz.build, so it ended up make-quoted in backend.mk
- That it's make-quoted in backend.mk is enough for the build to work

The rest of the issue is related to make showbuild and what the compilation database backend was doing, and that is obviously not exercised in most cases.

Also, most flags use characters that don't need make-quoting.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Add an expand_variables helper function to do simple make-like variable expansion

Turn out I'm using this function elsewhere in a WIP, so might as well put it in an importable module (gyp_reader is not). This addresses your comment about re.split.

Comment 13

[Mike Hommey [:glandium]]

Attachment: Better handle make variable references from gyp files

Comment 14

[Mike Hommey [:glandium]]

Attachment: Use mozbuild.shellutil.split instead of shlex.shlex in mozbuild.compilation.util

Turns out I had forgotten to add a hunk to this patch.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8691752 [details] [diff] [review]
Use mozbuild.shellutil.split instead of shlex.shlex in mozbuild.compilation.util

Review of attachment 8691752 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/compilation/util.py
@@ +48,3 @@
>  
> +    if not args:
> +        return []

This doesn't appear to be needed. But meh.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdca49c496fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50af5d1171b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d671ea9e4b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4038eff85b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb94204b1c96
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8a04f388bf1

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bdca49c496fa
https://hg.mozilla.org/mozilla-central/rev/f50af5d1171b
https://hg.mozilla.org/mozilla-central/rev/4d671ea9e4b3
https://hg.mozilla.org/mozilla-central/rev/a4038eff85b9
https://hg.mozilla.org/mozilla-central/rev/eb94204b1c96
https://hg.mozilla.org/mozilla-central/rev/f8a04f388bf1