Closed Bug 1224452 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(6 files, 2 obsolete files)

No description provided.
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.
Depends on: 1151124
Also make it only quote when actually necessary.
Attachment #8691178 - Flags: review?(gps)
And then properly quote the strings when printing them out in the compilation
database or the mach compileflags output.
Attachment #8691179 - Flags: review?(gps)
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.
Attachment #8691180 - Flags: review?(gps)
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.
Attachment #8691181 - Flags: review?(gps)
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.
Attachment #8691182 - Flags: review?(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.
Attachment #8691178 - Flags: review?(gps) → review+
Attachment #8691179 - Flags: review?(gps) → review+
Attachment #8691180 - Flags: review?(gps) → review+
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.
Attachment #8691182 - Flags: review?(gps) → review+
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!
Attachment #8691181 - Flags: review?(gps)
(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 .*"
(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.
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.
Attachment #8691726 - Flags: review?(gps)
Turns out I had forgotten to add a hunk to this patch.
Attachment #8691179 - Attachment is obsolete: true
Attachment #8691752 - Flags: review?(gps)
Attachment #8691726 - Flags: review?(gps) → review+
Attachment #8691727 - Flags: review?(gps) → review+
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.
Attachment #8691752 - Flags: review?(gps) → review+
Depends on: 1242132
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.