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)
Firefox Build System
General
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 |
Bug 1386876 - Replace all uses of DISABLE_STL_WRAPPING with a template, remove DISABLE_STL_WRAPPING.
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
mozreview-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/#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-
Comment 23•7 years ago
|
||
As a matter of fact, test_emitter.py and test_recursivemake.py still pass if I back it out.
Comment 24•7 years ago
|
||
mozreview-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/#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 25•7 years ago
|
||
mozreview-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/#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 26•7 years ago
|
||
mozreview-review |
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 27•7 years ago
|
||
mozreview-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)
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 29•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 30•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8898507 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8898508 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8898509 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8898510 -
Attachment is obsolete: true
Comment 37•7 years ago
|
||
mozreview-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 ::: 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 38•7 years ago
|
||
mozreview-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 39•7 years ago
|
||
mozreview-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 40•7 years ago
|
||
mozreview-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 41•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 42•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 49•7 years ago
|
||
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
Comment 50•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e1eaee9c428c https://hg.mozilla.org/mozilla-central/rev/97f3a6f21087 https://hg.mozilla.org/mozilla-central/rev/40dbde701504 https://hg.mozilla.org/mozilla-central/rev/562b0ea03828 https://hg.mozilla.org/mozilla-central/rev/2de74e372331 https://hg.mozilla.org/mozilla-central/rev/ff40df887048
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 51•7 years ago
|
||
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
Comment 52•7 years ago
|
||
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
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•