Closed Bug 1386876 Opened 7 years ago Closed 7 years ago

Implement STL_FLAGS and VISIBILITY_FLAGS as templates in moz.build

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(6 files, 4 obsolete files)

59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
      No description provided.
I have some more patches in this series (up to INCLUDES). I'll upload them here in case they're useful for context, although they probably should land in a different bug.
Comment on attachment 8893644 [details]
Bug 1386876 - Do not provide initial values for COMPILE_FLAGS when invoking within a template.

https://reviewboard.mozilla.org/r/164582/#review175196

::: python/mozbuild/mozbuild/frontend/reader.py:389
(Diff revision 3)
> +            # If a template wants to update a variable that's a dictionary we
> +            # need to pass our initial value down to ensure multiple templates
> +            # see our updates.
> +            for var, val in self._context.items():
> +                if var.isupper() and isinstance(val, dict):
> +                    context[var] = val

I'm still not sure this isn't going to hit us badly down the road. But let's assume it doesn't. That still leaves us with:
- the merging happening after the template execution a few lines below is now redundant for dicts.
- now dicts are somehow special compared to other types like lists, and let's not even talk about other simpler types.

Now the interesting thing is that I /think/ your patch series should work the same without this patch at all, because of the update() on dicts doing what you actually want with those COMPILE_FLAGS. Where it will fail is if you want to /add/ flags to them, but afaict, you don't do that yet, and I'm willing to say, let's wait until we actually need to.
Attachment #8893644 - Flags: review?(mh+mozilla) → review-
As a matter of fact, test_emitter.py and test_recursivemake.py still pass if I back it out.
Comment on attachment 8893645 [details]
Bug 1386876 - Add classes to handle compile flags computed by moz.build with templates, convert 'DISABLE_STL_WRAPPING' to use them.

https://reviewboard.mozilla.org/r/164584/#review175200

::: python/mozbuild/mozbuild/frontend/context.py:300
(Diff revision 3)
>              self[define] = 1
>          if value:
>              self.update(value)
>  
>  
> +class CompileFlags(ContextDerivedValue, dict):

This class is probably not limited enough. There's nothing preventing arbitrary keys to be set to weird values, and the known keys are also not type-checked.

::: python/mozbuild/mozbuild/frontend/context.py:302
(Diff revision 3)
>              self.update(value)
>  
>  
> +class CompileFlags(ContextDerivedValue, dict):
> +    def __init__(self, context):
> +        self.flag_variables = [

make that a tuple instead of a list.

::: python/mozbuild/mozbuild/frontend/context.py:305
(Diff revision 3)
> +class CompileFlags(ContextDerivedValue, dict):
> +    def __init__(self, context):
> +        self.flag_variables = [
> +            ('STL', (context.config.substs.get('STL_FLAGS'), ['CXXFLAGS'])),
> +        ]
> +        dict.__init__(self, [

It should work without creating an intermediate list: just pass the raw generator.

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py:403
(Diff revision 3)
>  
>      def test_program(self):
>          reader = self.reader('program')
>          objs = self.read_topsrcdir(reader)
>  
> +        objs = objs[1:]

here and below, you should instead check that you're getting a ComputedFlags, as expected.
Attachment #8893645 - Flags: review?(mh+mozilla)
Comment on attachment 8893646 [details]
Bug 1386876 - Replace all uses of DISABLE_STL_WRAPPING with a template, remove DISABLE_STL_WRAPPING.

https://reviewboard.mozilla.org/r/164586/#review175202

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 3)
>  
>             FINAL_TARGET_FILES += ['foo.png']
>             FINAL_TARGET_FILES.images['do-not-use'] += ['bar.svg']
>          """),
>  
> -    'DISABLE_STL_WRAPPING': (bool, bool,

Please add a deprecation not in DEPRECATION_HINTS

::: python/mozbuild/mozbuild/test/backend/data/variable_passthru/moz.build
(Diff revision 3)
>  LDFLAGS += ['-ld flag with spaces', '-x']
>  HOST_CFLAGS += ['-funroll-loops', '-wall']
>  HOST_CXXFLAGS += ['-funroll-loops-harder', '-wall-day-everyday']
>  WIN32_EXE_LDFLAGS += ['-subsystem:console']
>  
> -DISABLE_STL_WRAPPING = True

This and the other change in variable_passthru test data belong to the previous patch.
Attachment #8893646 - Flags: review?(mh+mozilla)
Comment on attachment 8893647 [details]
Bug 1386876 - Move visibility flag handling to computed compile flags with templates.

https://reviewboard.mozilla.org/r/164588/#review175206
Attachment #8893647 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8893648 [details]
Bug 1386876 - Replace all uses of NO_VISIBILITY_FLAGS with a template and remove NO_VISIBILITY_FLAGS.

https://reviewboard.mozilla.org/r/164590/#review175208

::: python/mozbuild/mozbuild/test/backend/data/variable_passthru/moz.build
(Diff revision 3)
>  # -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-
>  # Any copyright is dedicated to the Public Domain.
>  # http://creativecommons.org/publicdomain/zero/1.0/
>  
> -NO_VISIBILITY_FLAGS = True

Same remark as for DISABLE_STL_WRAPPERS, the variable_passthru test changes belong to the previous patch.
Attachment #8893648 - Flags: review?(mh+mozilla)
Comment on attachment 8893644 [details]
Bug 1386876 - Do not provide initial values for COMPILE_FLAGS when invoking within a template.

https://reviewboard.mozilla.org/r/164582/#review175196

> I'm still not sure this isn't going to hit us badly down the road. But let's assume it doesn't. That still leaves us with:
> - the merging happening after the template execution a few lines below is now redundant for dicts.
> - now dicts are somehow special compared to other types like lists, and let's not even talk about other simpler types.
> 
> Now the interesting thing is that I /think/ your patch series should work the same without this patch at all, because of the update() on dicts doing what you actually want with those COMPILE_FLAGS. Where it will fail is if you want to /add/ flags to them, but afaict, you don't do that yet, and I'm willing to say, let's wait until we actually need to.

Unfortunately this is necessary... if we use both `DisableStlWrapping` and `NoVisibilityFlags`, whichever comes second negates the first, because the `update` provides the default value from the template's context and destroys the fact the prior template cleared out that value.

I'll think about a better way to achieve this and add a test case to demonstrate the issue here.
Comment on attachment 8893644 [details]
Bug 1386876 - Do not provide initial values for COMPILE_FLAGS when invoking within a template.

https://reviewboard.mozilla.org/r/164582/#review175196

> Unfortunately this is necessary... if we use both `DisableStlWrapping` and `NoVisibilityFlags`, whichever comes second negates the first, because the `update` provides the default value from the template's context and destroys the fact the prior template cleared out that value.
> 
> I'll think about a better way to achieve this and add a test case to demonstrate the issue here.

Interestingly there's an instance of this issue possible today (although not probably and issue in practice) with `DEFINES`: if we have `DEBUG` in `MOZ_DEBUG_DEFINES`, one template that sets `DEFINES['DEBUG'] = False` and a later one that sets an unrelated value in `DEFINES`, both of which are invoked in the same context, the emitted defines will contain `-DDEBUG=1`. I wouldn't really expect someone to try this, but we don't prevent the scenario, and emit `-UDEBUG` in the absence of the second template.

To avoid special casing dictionaries we might be better off populating the template's context with every variable present in the invoking file's context before expanding the template and re-populating the invoking file's context with the same afterward so that any modifications to mutable variables in the template work as expected.
Comment on attachment 8893644 [details]
Bug 1386876 - Do not provide initial values for COMPILE_FLAGS when invoking within a template.

https://reviewboard.mozilla.org/r/164582/#review175196

> Interestingly there's an instance of this issue possible today (although not probably and issue in practice) with `DEFINES`: if we have `DEBUG` in `MOZ_DEBUG_DEFINES`, one template that sets `DEFINES['DEBUG'] = False` and a later one that sets an unrelated value in `DEFINES`, both of which are invoked in the same context, the emitted defines will contain `-DDEBUG=1`. I wouldn't really expect someone to try this, but we don't prevent the scenario, and emit `-UDEBUG` in the absence of the second template.
> 
> To avoid special casing dictionaries we might be better off populating the template's context with every variable present in the invoking file's context before expanding the template and re-populating the invoking file's context with the same afterward so that any modifications to mutable variables in the template work as expected.

Actually, I think we just need to work around this by not providing default values for COMPUTED_FLAGS in a template.
Attachment #8898507 - Attachment is obsolete: true
Attachment #8898508 - Attachment is obsolete: true
Attachment #8898509 - Attachment is obsolete: true
Attachment #8898510 - Attachment is obsolete: true
Comment on attachment 8893645 [details]
Bug 1386876 - Add classes to handle compile flags computed by moz.build with templates, convert 'DISABLE_STL_WRAPPING' to use them.

https://reviewboard.mozilla.org/r/164584/#review182114

::: python/mozbuild/mozbuild/frontend/context.py:303
(Diff revision 4)
>  
>  
> +class CompileFlags(ContextDerivedValue, dict):
> +    def __init__(self, context):
> +        self.flag_variables = (
> +            ('STL', (context.config.substs.get('STL_FLAGS'), ['CXXFLAGS'])),

you could save an indirection by flattening the tuples.

::: python/mozbuild/mozbuild/frontend/context.py:303
(Diff revision 4)
>  
>  
> +class CompileFlags(ContextDerivedValue, dict):
> +    def __init__(self, context):
> +        self.flag_variables = (
> +            ('STL', (context.config.substs.get('STL_FLAGS'), ['CXXFLAGS'])),

It would be better for CXXFLAGS to be in a tuple too.

::: python/mozbuild/mozbuild/frontend/context.py:311
(Diff revision 4)
> +
> +        dict.__init__(self,
> +                      ((k, TypedList(unicode)(v)) for k, (v, _) in self.flag_variables))
> +
> +    def __setitem__(self, key, value):
> +        if key not in self._known_keys:

I think this can work without self._known_keys, if you just do if key not in self.

::: python/mozbuild/mozbuild/frontend/context.py:314
(Diff revision 4)
> +
> +    def __setitem__(self, key, value):
> +        if key not in self._known_keys:
> +            raise ValueError('Invalid value. `%s` is not a compile flags '
> +                             'category.' % key)
> +        if not (isinstance(value, list) and all([isinstance(v, unicode) for v in value])):

no need to create an intermediate list here. all(isinstance(...) for ...) will do.

::: python/mozbuild/mozbuild/frontend/gyp_reader.py:329
(Diff revision 4)
>            ]
>            # These get set via VC project file settings for normal GYP builds.
>            if config.substs['OS_TARGET'] == 'WINNT':
>                context['DEFINES']['UNICODE'] = True
>                context['DEFINES']['_UNICODE'] = True
> -        context['DISABLE_STL_WRAPPING'] = True
> +        context['COMPILE_FLAGS']['STL'] = []

random thought: maybe `del context['COMPILE_FLAGS']['STL']` would be nicer (here and in the template)

::: python/mozbuild/mozbuild/test/common.py:38
(Diff revision 4)
> -        self.substs_unicode = ReadOnlyDict({k.decode('utf-8'): v.decode('utf-8',
> -            'replace') for k, v in self.substs.items()})
> +        def decode_value(value):
> +            if isinstance(value, list):
> +                return [v.decode('utf-8', 'replace') for v in value]
> +            return value.decode('utf-8', 'replace')
> +
> +        self.substs_unicode = ReadOnlyDict({k.decode('utf-8'): decode_value(v)

It would be better for this and the AC_SUBST_LIST change to go in a separate patch.

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py:208
(Diff revision 4)
>          self.assertEqual(wanted, variables)
>          self.maxDiff = maxDiff
>  
> +    def test_compile_flags(self):
> +        reader = self.reader('compile-flags',
> +                             extra_substs={'STL_FLAGS': ['-I${DIST}/stl_wrappers']})

Note that in old-configure, STL_FLAGS is set to "-I${DIST}/stl_wrappers", which means ${DIST} is expanded at the shell level.

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py:1085
(Diff revision 4)
>          reader = self.reader('rust-library-dash-folding',
>                               extra_substs=dict(RUST_TARGET='i686-pc-windows-msvc'))
>          objs = self.read_topsrcdir(reader)
>  
> -        self.assertEqual(len(objs), 1)
> -        lib = objs[0]
> +        self.assertEqual(len(objs), 2)
> +        lib = objs[1]

here and below, you should also check that obj[0] is a ComputedFlags.
Attachment #8893645 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8893646 [details]
Bug 1386876 - Replace all uses of DISABLE_STL_WRAPPING with a template, remove DISABLE_STL_WRAPPING.

https://reviewboard.mozilla.org/r/164586/#review182120

Not sure why some were in the first patch.
Attachment #8893646 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8898506 [details]
Bug 1386876 - Allow dictionary variables to be passed to gyp_dir_attrs.

https://reviewboard.mozilla.org/r/169838/#review182124

r+ with the change in this file from next patch folded in.
Attachment #8898506 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8893648 [details]
Bug 1386876 - Replace all uses of NO_VISIBILITY_FLAGS with a template and remove NO_VISIBILITY_FLAGS.

https://reviewboard.mozilla.org/r/164590/#review182122

::: python/mozbuild/mozbuild/frontend/gyp_reader.py:338
(Diff revision 4)
>                  # If we have a key from sanbox_vars that's also been
>                  # populated here we use the value from sandbox_vars as our
>                  # basis rather than overriding outright.
>                  context[key] = value + context[key]
>              elif context.get(key) and isinstance(context[key], dict):
> -                context.update(value)
> +                context[key].update(value)

this should be in the previous patch.
Attachment #8893648 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8893644 [details]
Bug 1386876 - Do not provide initial values for COMPILE_FLAGS when invoking within a template.

https://reviewboard.mozilla.org/r/164582/#review182126

Kind of gross, but meh. That invalidates my comment about possibly working without self._known_keys.
Attachment #8893644 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8893645 [details]
Bug 1386876 - Add classes to handle compile flags computed by moz.build with templates, convert 'DISABLE_STL_WRAPPING' to use them.

https://reviewboard.mozilla.org/r/164584/#review182114

> I think this can work without self._known_keys, if you just do if key not in self.

Unfortunately that doesn't work with the final patch in this series because that patch makes COMPILE_FLAGS objects empty when accessed from within a template.

> random thought: maybe `del context['COMPILE_FLAGS']['STL']` would be nicer (here and in the template)

I think I actually tried this first, but it doesn't work for the template, because the merge after the template runs just consists of calling `update` with the value for COMPILE_FLAGS we get from the template, so the fact that 'STL' or whatever key has been deleted in the template will not be reflected in the resulting context.
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1eaee9c428c
Add classes to handle compile flags computed by moz.build with templates, convert 'DISABLE_STL_WRAPPING' to use them. r=glandium
https://hg.mozilla.org/integration/autoland/rev/97f3a6f21087
Replace all uses of DISABLE_STL_WRAPPING with a template, remove DISABLE_STL_WRAPPING. r=glandium
https://hg.mozilla.org/integration/autoland/rev/40dbde701504
Move visibility flag handling to computed compile flags with templates. r=glandium
https://hg.mozilla.org/integration/autoland/rev/562b0ea03828
Allow dictionary variables to be passed to gyp_dir_attrs. r=glandium
https://hg.mozilla.org/integration/autoland/rev/2de74e372331
Replace all uses of NO_VISIBILITY_FLAGS with a template and remove NO_VISIBILITY_FLAGS. r=glandium
https://hg.mozilla.org/integration/autoland/rev/ff40df887048
Do not provide initial values for COMPILE_FLAGS when invoking within a template. r=glandium
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c21facbcd3ef
Port 1386876 to C-C: s/NO_VISIBILITY_FLAGS = True/NoVisibilityFlags()/. rs=bustage-fix
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0eec5f47782d
Port 1386876 to C-C: s/DISABLE_STL_WRAPPING = True/DisableStlWrapping()/. rs=bustage-fix
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.