Closed Bug 1398897 Opened 2 years ago Closed 2 years ago

Move DEFINES and INCLUDES to moz.build computed flags

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: cmanchester, Assigned: cmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

No description provided.
Comment on attachment 8907296 [details]
Bug 1398897 - Move defines to computed compile flags.

https://reviewboard.mozilla.org/r/178920/#review184830

::: python/mozbuild/mozbuild/frontend/data.py:175
(Diff revision 1)
> +    def resolve_flags(self, key, value):
> +        if key in self.flags and self.flags[key]:
> +            raise ResolvedFlagsError(
> +                "'%s' may not be set in COMPILE_FLAGS from moz.build or with "
> +                "an initial value, this value is resolved from the emitter." %
> +                key)
> +        self.flags[key] = value
> +

I think it would be better to do this check from CompileFlags itself. And ComputedFlags can bypass the check by using dict.__setitem__(self.flags, key, value)

::: python/mozbuild/mozbuild/test/frontend/data/resolved-flags-error/moz.build:14
(Diff revision 1)
> +Library('dummy')
> +
> +UNIFIED_SOURCES += ['test1.c']
> +
> +DEFINES['MOZ_TEST_DEFINE'] = True
> +LIBRARY_DEFINES['MOZ_TEST_DEFINE'] = 'MOZ_TEST'

wouldn't it be better to define something different in LIBRARY_DEFINES than in DEFINES? BTW, shouldn't defining the same thing in both be considered an error?

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py:904
(Diff revision 1)
> +        flags = objs.pop()
> +        self.assertIsInstance(flags, ComputedFlags)

should be above the comment
Attachment #8907296 - Flags: review?(mh+mozilla)
Comment on attachment 8907297 [details]
Bug 1398897 - Move includes to computed flags.

https://reviewboard.mozilla.org/r/178922/#review184836

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:756
(Diff revision 1)
>      def test_rust_library(self):
>          """Test that a Rust library is written to backend.mk correctly."""
>          env = self._consume('rust-library', RecursiveMakeBackend)
>  
>          backend_path = mozpath.join(env.topobjdir, 'backend.mk')
> -        lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:]]
> +        lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:-2]]

wouldn't it be better to add the extra lines to `expected`?

It could even help decide whether we actually should end up with those lines in cases like this one, where only rust code is involved.
Attachment #8907297 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907298 [details]
Bug 1398897 - Move breakpad include munging to moz.build

https://reviewboard.mozilla.org/r/178924/#review184840
Attachment #8907298 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907299 [details]
Bug 1398897 - Move includes associated with CPP_UNIT_TESTS to the CppUnitTests template.

https://reviewboard.mozilla.org/r/178926/#review184844
Attachment #8907299 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907300 [details]
Bug 1398897 - Move os includes to computed flags.

https://reviewboard.mozilla.org/r/178928/#review184846

Note that this will generate a lot of repeted churn in all the backend makefiles... :-/

::: python/mozbuild/mozbuild/frontend/context.py:318
(Diff revision 1)
>              ('EXTRA_INCLUDES', ['-I%s/dist/include' % context.config.topobjdir],
>               ('CXXFLAGS', 'CFLAGS')),
> +            ('OS_INCLUDES', list(itertools.chain(*(context.config.substs.get(v, []) for v in (
> +                'NSPR_CFLAGS', 'NSS_CFLAGS', 'MOZ_JPEG_CFLAGS', 'MOZ_PNG_CFLAGS',
> +                'MOZ_ZLIB_CFLAGS', 'MOZ_PIXMAN_CFLAGS')))),
> +             ('CXXFLAGS', 'CFLAGS')),

aaah, I just realized that here and for the other flags moved in this bug, it's probably time we finally do the right thing and put then in CPPFLAGS instead of CFLAGS/CXXFLAGS.
Attachment #8907300 - Flags: review?(mh+mozilla)
Comment on attachment 8907297 [details]
Bug 1398897 - Move includes to computed flags.

https://reviewboard.mozilla.org/r/178922/#review184836

> wouldn't it be better to add the extra lines to `expected`?
> 
> It could even help decide whether we actually should end up with those lines in cases like this one, where only rust code is involved.

We can pretty simply avoid emitting these flags for dirs only containing rust libraries, I'll add that as a pre patch.
Comment on attachment 8907300 [details]
Bug 1398897 - Move os includes to computed flags.

https://reviewboard.mozilla.org/r/178928/#review184846

> aaah, I just realized that here and for the other flags moved in this bug, it's probably time we finally do the right thing and put then in CPPFLAGS instead of CFLAGS/CXXFLAGS.

This seems reasonable, but for a first pass I'm hoping to maintain the order of the flags to simplify verifying the result of these patches, and I don't think there would be a straightforward way to do that if we split the backend variables between C{XX}FLAGS and CPPFLAGS.
Comment on attachment 8907296 [details]
Bug 1398897 - Move defines to computed compile flags.

https://reviewboard.mozilla.org/r/178920/#review186752
Attachment #8907296 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8908743 [details]
Bug 1398897 - Do not emit compile flags for directories only containing rust libraries.

https://reviewboard.mozilla.org/r/180368/#review186754

::: python/mozbuild/mozbuild/frontend/emitter.py:766
(Diff revision 1)
>  
> +        # Avoid emitting compile flags for directories only containing rust
> +        # libraries. Emitted compile flags are only relevant to C/C++ sources
> +        # for the time being.
> +        if not all(isinstance(l, (RustLibrary, HostRustLibrary)) for l in
> +                   linkables + host_linkables):

might as well put the `for` on this line.
Attachment #8908743 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907300 [details]
Bug 1398897 - Move os includes to computed flags.

https://reviewboard.mozilla.org/r/178928/#review186756

Please file a followup for the CFLAGS/CXXFLAGS->CPPFLAGS transition.
Attachment #8907300 - Flags: review?(mh+mozilla) → review+
Blocks: 1401664
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82f0e87bd0ef
Do not emit compile flags for directories only containing rust libraries. r=glandium
https://hg.mozilla.org/integration/autoland/rev/300708364bfb
Move defines to computed compile flags. r=glandium
https://hg.mozilla.org/integration/autoland/rev/9b5a00094c49
Move includes to computed flags. r=glandium
https://hg.mozilla.org/integration/autoland/rev/ae4d0410545f
Move breakpad include munging to moz.build r=glandium
https://hg.mozilla.org/integration/autoland/rev/e4ac1e17ffe5
Move includes associated with CPP_UNIT_TESTS to the CppUnitTests template. r=glandium
https://hg.mozilla.org/integration/autoland/rev/b4c11ce9704c
Move os includes to computed flags. r=glandium
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.