Closed Bug 1362612 Opened 7 years ago Closed 7 years ago

Consolidate compilation flag handling for consumption by multiple build backends

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(3 obsolete files) 

This is a step towards bug 1319563 but focused on flag handling so the remaining work for backends to build command lines is trivial.

The idea is to incorporate the logic from the compile db bit by bit into the emitter and use the flags calculated by the emitter in both the compile db and the recursive make backend to prevent divergence as we port things over. Some things might be less awkward if we move them to the common backend rather than the emitter, but the emitter is a fine place as a proof of concept. I have a stack of patches that get us there for compilation flags to the extent I can generate the same compile db as before the patches and generate builds on most platforms. There are still some significant pieces to work out like handling host compilation and the edge case directories that aren't dealt with by the compile db (due to setting flags in Makefile.in), but I'll post some for feedback to see if we can agree on the approach.
Attachment #8865064 - Flags: feedback?(mshal)
Attachment #8865064 - Flags: feedback?(mh+mozilla)
Attachment #8865065 - Flags: feedback?(mshal)
Attachment #8865065 - Flags: feedback?(mh+mozilla)
Attachment #8865066 - Flags: feedback?(mshal)
Attachment #8865066 - Flags: feedback?(mh+mozilla)
Comment on attachment 8865064 [details]
Bug 1362612 - Add classes to handle compile flags computed by moz.build with templates, convert 'DISABLE_STL_WRAPPING' to use them.

https://reviewboard.mozilla.org/r/136742/#review141294

Overall I like the approach! I wasn't expected us to tackle matching the RecursiveMake backend as we go, but this seems like a good way to do so (and will probably result in fewer surprises). I'm curious if this lines up with what glandium was thinking.

::: python/mozbuild/mozbuild/frontend/emitter.py:969
(Diff revision 1)
>          if dist_install is True:
>              passthru.variables['DIST_INSTALL'] = True
>          elif dist_install is False:
>              passthru.variables['NO_DIST_INSTALL'] = True
>  
> +        computed_flags = ComputedFlags(context)

This section will grow fairly large as things are moved into ComputedFlags, correct? (eg: large portions of config.mk ported into python). If so I think it would make sense to move this into a separate file, or at least its own function, so that the mapping from moz.build data to compiler command-line is relatively self-contained.
Comment on attachment 8865065 [details]
Bug 1362612 - Move visibility flag handling to computed compile flags.

https://reviewboard.mozilla.org/r/136744/#review141300

::: python/mozbuild/mozbuild/frontend/emitter.py:977
(Diff revision 1)
> +                                                      ('VISIBILITY_FLAGS', ('CXXFLAGS', 'CFLAGS')))):
> +            if not context[disable_var]:
> +                flags = context.config.substs.get(config_var)
> +                if flags:
> +                    if isinstance(flags, basestring):
> +                        flags = shlex.split(flags)

Is this the right thing to do? It might be easier to just AC_SUBST_LIST(STL_FLAGS) in old-configure.in rather than support both lists & strings.
Comment on attachment 8865066 [details]
Bug 1362612 - Move defines to computed compile flags.

https://reviewboard.mozilla.org/r/136746/#review141310

::: python/mozbuild/mozbuild/frontend/emitter.py:1033
(Diff revision 1)
> +                # If we don't have explicitly set defines we need to make sure
> +                # initialized values if present end up in computed flags.
> +                defines_obj = cls(context, context[defines_var])
> +
> +            if isinstance(defines_obj, Defines):
> +                defines_from_obj = list(defines_obj.get_defines())

Would it help here if we added MOZ_DEBUG_DEFINES to the computed flags directly? I wonder if they still need to be in DEFINES at all anymore (for preprocessing or whatever) if those flags are now in COMPUTED_FLAGS.
Attachment #8865066 - Flags: feedback?(mshal) → feedback+
Attachment #8865065 - Flags: feedback?(mshal) → feedback+
Attachment #8865064 - Flags: feedback?(mshal) → feedback+
Comment on attachment 8865064 [details]
Bug 1362612 - Add classes to handle compile flags computed by moz.build with templates, convert 'DISABLE_STL_WRAPPING' to use them.

https://reviewboard.mozilla.org/r/136742/#review141294

> This section will grow fairly large as things are moved into ComputedFlags, correct? (eg: large portions of config.mk ported into python). If so I think it would make sense to move this into a separate file, or at least its own function, so that the mapping from moz.build data to compiler command-line is relatively self-contained.

Yeah, things do get a little messy here. I'm looking in to a slightly more declarative way to represent how we build out the flags.
Comment on attachment 8865065 [details]
Bug 1362612 - Move visibility flag handling to computed compile flags.

https://reviewboard.mozilla.org/r/136744/#review141300

> Is this the right thing to do? It might be easier to just AC_SUBST_LIST(STL_FLAGS) in old-configure.in rather than support both lists & strings.

Right, I guess we should move as much as possible to use lists.
Comment on attachment 8865066 [details]
Bug 1362612 - Move defines to computed compile flags.

https://reviewboard.mozilla.org/r/136746/#review141310

> Would it help here if we added MOZ_DEBUG_DEFINES to the computed flags directly? I wonder if they still need to be in DEFINES at all anymore (for preprocessing or whatever) if those flags are now in COMPUTED_FLAGS.

Yeah, that's probably a little more explicit. I see instances of `ifdef DEBUG` in .js files.
Sorry for the delay. My gut reaction here is that this could work but it feels like a lot of logic moving around from backend to frontend, and I'm wondering if we wouldn't be better off on the long term if we could avoid all that in either of them in the first place. I'm not sure for DEFINES, but is seems like STL_FLAGS and VISIBILITY_FLAGS would be prime candidates for being templatized in build/*templates.mozbuild, filling CFLAGS/CXXFLAGS as appropriate.
Attachment #8865064 - Flags: feedback?(mh+mozilla)
Attachment #8865065 - Flags: feedback?(mh+mozilla)
Attachment #8865066 - Flags: feedback?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #10)
> Sorry for the delay. My gut reaction here is that this could work but it
> feels like a lot of logic moving around from backend to frontend, and I'm
> wondering if we wouldn't be better off on the long term if we could avoid
> all that in either of them in the first place. I'm not sure for DEFINES, but
> is seems like STL_FLAGS and VISIBILITY_FLAGS would be prime candidates for
> being templatized in build/*templates.mozbuild, filling CFLAGS/CXXFLAGS as
> appropriate.

Looking at these I'm having trouble seeing how they can be moved to templates. I think this would either mean converting "DISABLE_STL_WRAPPING = True" to a template itself ("DisableStlWrapping()" in its place), or making stl_flags a parameter to Library, Program, etc. that would populate them if necessary for sources contributing to that binary. For the former this is clumsy because in these cases we're setting something that excludes flags that are included in most directories (we'd still need some logic somewhere that says STL_FLAGS ends up in CXXFLAGS by default), and for the latter it looks like a template can't always stand in for these (there are cases we have NO_VISIBILITY_FLAGS set in a moz.build but there isn't a template present in the file, but just FINAL_LIBRARY).

Can you explain a bit what you have in mind? Thank you.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8865064 [details]
Bug 1362612 - Add classes to handle compile flags computed by moz.build with templates, convert 'DISABLE_STL_WRAPPING' to use them.

https://reviewboard.mozilla.org/r/136742/#review141294

> Yeah, things do get a little messy here. I'm looking in to a slightly more declarative way to represent how we build out the flags.

I updated this with a somewhat different approach putting a little more burden on the flags object in data.py to define a "scheme" for flags that things from the emitter can contribute to. This is still a little messy, because the variables here don't really correspond to anything other than how things are in the recursivemake backend, but I think it's an improvement because it makes it a little more obvious how things in the emitter contribute to flags and it still keeps logic for flags out of individual backends.
Attachment #8865065 - Attachment is obsolete: true
Attachment #8865066 - Attachment is obsolete: true
Comment 16 is a version of moving a flag with templates. I'm not sure if this is less awkward than the prior approach, but may be useful for discussion.
(In reply to Chris Manchester (:chmanchester) from comment #17)
> Comment 16 is a version of moving a flag with templates. I'm not sure if
> this is less awkward than the prior approach, but may be useful for
> discussion.

This is actually problematic due to how templates work if multiple templates attempt to modify COMPILE_FLAGS. If we have:

> @template
> def DisableStlWrapping():
>     COMPILE_FLAGS['STL_FLAGS'] = []

> @template
> def NoVisibilityFlags():
>     COMPILE_FLAGS['VISIBILITY_FLAGS'] = []

And a moz.build with both `DisableStlWrapping` and `NoVisibilityFlags`, the emitter sees two `COMPILE_FLAGS` data structures, one with `STL_FLAGS` removed and one with `VISIBILITY_FLAGS` removed, combining both of which negates the effect of both.
Flags: needinfo?(mh+mozilla)
I have some initial patches that can be landed, I'll spin those off into a dependent bug.
Depends on: 1386876
Blocks: 1408675
Attachment #8865064 - Attachment is obsolete: true
This is essentially fixed in dependent bugs. We'll file follow ups for odds and ends we find as we're implementing other backends.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: