Closed
Bug 1319222
Opened 9 years ago
Closed 8 years ago
Support compilation objects in the tup backend
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: mshal, Assigned: mshal)
References
Details
Attachments
(10 files)
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
We'll need to compile all of the *Sources moz.build variables in the tup backend. I plan to share most of the CompilationDB command-line generation logic in the CommonBackend, and use that for the tup backend.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mshal
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 10•8 years ago
|
||
With this set of patches, a build with the tup backend build will generate the same set of files as a recursive make build with an empty mozconfig. I haven't yet verified that the object files are identical, but progress! And actually linking them will be handled in other bugs.
I requested chmanchester for the flags patches since I'm really not sure if those are implemented correctly. They seemed to be required for host and asm compilation in a few places.
Updated•8 years ago
|
Attachment #8931140 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931141 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931142 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931144 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931145 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931147 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931148 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8931140 [details]
Bug 1319222 - Remove srcdir from BackendTupfile;
https://reviewboard.mozilla.org/r/202222/#review207582
Attachment #8931140 -
Flags: review?(cmanchester) → review+
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8931141 [details]
Bug 1319222 - Use relobjdir instead of relativedir to determine outputs;
https://reviewboard.mozilla.org/r/202224/#review207584
::: python/mozbuild/mozbuild/backend/tup.py:207
(Diff revision 1)
> BackendTupfile(objdir, self.environment,
> self.environment.topsrcdir, self.environment.topobjdir)
> return self._backend_files[objdir]
>
> def _get_backend_file_for(self, obj):
> - return self._get_backend_file(obj.relativedir)
> + return self._get_backend_file(obj.relobjdir)
Wow, this is sort of a footgun. I would say let's re-name `relativedir` to `relsrcdir`... in the constructor for ContextDerived we actually do `self.relativedir = context.relsrcdir`. If you'd rather not fix that here let's do it in a follow up.
I guess this explains all the times I've failed trying to find `relsrcdir` on an object in the build system.
Attachment #8931141 -
Flags: review?(cmanchester) → review+
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8931142 [details]
Bug 1319222 - Change --param flag to be multiple arguments;
https://reviewboard.mozilla.org/r/202226/#review207586
Attachment #8931142 -
Flags: review?(cmanchester) → review+
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8931143 [details]
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;
https://reviewboard.mozilla.org/r/202228/#review207590
::: python/mozbuild/mozbuild/frontend/context.py:353
(Diff revision 1)
> + ('DEFINES', None, ('ASFLAGS',)),
> + ('LIBRARY_DEFINES', None, ('ASFLAGS',)),
> + ('LOCAL_INCLUDES', None, ('ASFLAGS',)),
There are two potential issues here:
DEFINES and INCLUDES are added to the command line with ASFLAGS for SOBJS (made from `*.S` files), but not for ASOBJS (made from `*.s` files). Also, by adding them here but not deleting them from the rules.mk command line we'd end up with them duplicated in the Make backend. Probably we can split these off into a separate variable if this distinction is really significant.
The second potential issue is that in the Make backend DEFINES come before ASFLAGS, followed by per-source flags, followed by INCLUDES. Changing the order could be significant. I doubt it is, but we would want to check. If we want to preserve the order we could probably just cobble this together in the backend.
::: python/mozbuild/mozbuild/frontend/emitter.py:1237
(Diff revision 1)
> if passthru.variables:
> yield passthru
>
> if context.objdir in self._compile_dirs:
> self._compile_flags[context.objdir] = computed_flags
> + self._compile_as_flags[context.objdir] = computed_as_flags
This would make `_asm_compile_dirs` obsolete. Was that too restrictive for some reason?
Attachment #8931143 -
Flags: review?(cmanchester)
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8931144 [details]
Bug 1319222 - Compile the generated IPDL and WebIDL sources in the tup backend;
https://reviewboard.mozilla.org/r/202230/#review207592
::: python/mozbuild/mozbuild/backend/tup.py:569
(Diff revision 1)
> cmd=cmd,
> outputs=outputs,
> extra_outputs=[self._installed_files],
> check_unchanged=True,
> )
> + backend_file.sources['.cpp'].extend([u[0] for u in unified_ipdl_cppsrcs_mapping])
Intermediate list not needed?
::: python/mozbuild/mozbuild/backend/tup.py:602
(Diff revision 1)
> inputs=webidls.all_non_static_basenames(),
> outputs=outputs,
> extra_outputs=[self._installed_files],
> check_unchanged=True,
> )
> + backend_file.sources['.cpp'].extend([u[0] for u in unified_source_mapping])
Intermediate list not needed?
Attachment #8931144 -
Flags: review?(cmanchester) → review+
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8931145 [details]
Bug 1319222 - Enable compilation on all directories in the tup backend;
https://reviewboard.mozilla.org/r/202232/#review207594
Attachment #8931145 -
Flags: review?(cmanchester) → review+
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8931146 [details]
Bug 1319222 - Add include flags to HOST_*;
https://reviewboard.mozilla.org/r/202234/#review207600
::: python/mozbuild/mozbuild/frontend/context.py:332
(Diff revision 1)
> + ('BASE_INCLUDES', ['-I%s' % main_src_dir, '-I%s' % context.objdir],
> + ('HOST_CFLAGS', 'HOST_CXXFLAGS')),
> + ('LOCAL_INCLUDES', None, ('HOST_CFLAGS', 'HOST_CXXFLAGS')),
> + ('EXTRA_INCLUDES', ['-I%s/dist/include' % context.config.topobjdir],
> + ('HOST_CFLAGS', 'HOST_CXXFLAGS')),
This has a sort of similar issue to `ASFLAGS`: the includes aren't passed every time we see `HOST_CFLAGS`, and we'd have to delete these from `rules.mk` if we were to add them here. Again we probably need to shunt these off to a separate variable or just stitch things together in the tup backend for now.
Attachment #8931146 -
Flags: review?(cmanchester)
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8931147 [details]
Bug 1319222 - Enable host compilation in the tup backend;
https://reviewboard.mozilla.org/r/202236/#review207602
::: python/mozbuild/mozbuild/backend/tup.py:122
(Diff revision 1)
> + s = self.sources
> + h = self.host_sources
Nit: one char variables are hard to grep for.
::: python/mozbuild/mozbuild/backend/tup.py:125
(Diff revision 1)
> - ('.S', 'AS', 'ASFLAGS'),
> - ('.cpp', 'CXX', 'CXXFLAGS'),
> - ('.c', 'CC', 'CFLAGS'),
> + (s['.S'], 'AS', 'ASFLAGS', ''),
> + (s['.cpp'], 'CXX', 'CXXFLAGS', ''),
> + (s['.c'], 'CC', 'CFLAGS', ''),
> + (h['.cpp'], 'HOST_CXX', 'HOST_CXXFLAGS', 'host_'),
> + (h['.c'], 'HOST_CC', 'HOST_CFLAGS', 'host_'),
I just noticed the recursivemake backend cares about the difference between `.s` and `.S`. Sorry, I probably should have caught this is a prior review :/
Attachment #8931147 -
Flags: review?(cmanchester) → review+
Comment 19•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931147 [details]
Bug 1319222 - Enable host compilation in the tup backend;
https://reviewboard.mozilla.org/r/202236/#review207602
> I just noticed the recursivemake backend cares about the difference between `.s` and `.S`. Sorry, I probably should have caught this is a prior review :/
Oh, you take care of this in the next patch. Nevermind!
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8931148 [details]
Bug 1319222 - Support compiling *.s with yasm;
https://reviewboard.mozilla.org/r/202238/#review207606
::: python/mozbuild/mozbuild/backend/tup.py:142
(Diff revision 1)
> - cmd.extend(['-c', '%f', '-o', '%o'])
> + if compiler == 'AS':
> + cmd.append(self.variables.get('AS_DASH_C_FLAG', '-c'))
> + else:
> + cmd.append('-c')
Can this whole thing just be `if compile_value != self.environment.substs['YASM']: cmd.append('-c')`?
That way we can avoid a reference to `AS_DASH_C_FLAG`.
Attachment #8931148 -
Flags: review?(cmanchester) → review+
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931141 [details]
Bug 1319222 - Use relobjdir instead of relativedir to determine outputs;
https://reviewboard.mozilla.org/r/202224/#review207584
> Wow, this is sort of a footgun. I would say let's re-name `relativedir` to `relsrcdir`... in the constructor for ContextDerived we actually do `self.relativedir = context.relsrcdir`. If you'd rather not fix that here let's do it in a follow up.
>
> I guess this explains all the times I've failed trying to find `relsrcdir` on an object in the build system.
Filed bug 1421038 as a followup.
Assignee | ||
Comment 22•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931148 [details]
Bug 1319222 - Support compiling *.s with yasm;
https://reviewboard.mozilla.org/r/202238/#review207606
> Can this whole thing just be `if compile_value != self.environment.substs['YASM']: cmd.append('-c')`?
>
> That way we can avoid a reference to `AS_DASH_C_FLAG`.
That does seem to work, though that makes this logic a little redundant with how AS_DASH_C_FLAG gets set in the emitter. I think it might be confusing to have that in the emitter and go unused in the tup backend.
I've re-worked it slightly to specify the dash_c setting in the compilers list so it no longer uses the weird if-statement. I'll ask for re-review on this one.
Assignee | ||
Comment 23•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931143 [details]
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;
https://reviewboard.mozilla.org/r/202228/#review207590
> There are two potential issues here:
>
> DEFINES and INCLUDES are added to the command line with ASFLAGS for SOBJS (made from `*.S` files), but not for ASOBJS (made from `*.s` files). Also, by adding them here but not deleting them from the rules.mk command line we'd end up with them duplicated in the Make backend. Probably we can split these off into a separate variable if this distinction is really significant.
>
> The second potential issue is that in the Make backend DEFINES come before ASFLAGS, followed by per-source flags, followed by INCLUDES. Changing the order could be significant. I doubt it is, but we would want to check. If we want to preserve the order we could probably just cobble this together in the backend.
Ahh, good catch. I agree it's probably easiest to separate the flags for SOBJS out into a separate variable. I've re-worked this commit to go with that approach, and verified locally that the before & after commandlines in the make backend are identical for a .s and a .S file.
> This would make `_asm_compile_dirs` obsolete. Was that too restrictive for some reason?
I think this issue is no longer relevant with the new approach, since the original AsmFlags are the same as before. Let me know if I'm misunderstanding.
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 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8931143 [details]
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;
https://reviewboard.mozilla.org/r/202228/#review209980
Looks good, thanks for doing this!
::: python/mozbuild/mozbuild/frontend/context.py:2034
(Diff revision 2)
> - """Recipe for linker flags for this context. Not to be manipulated
> - directly.
> + """Recipe for assembler flags for this context, without define or
> + include flags since they are not supported by some assemblers. Not to
This comment isn't quite right now (I don't think it needs to be updated). The `ASM_FLAGS` dict will have everything but decide which flags categories to pass on to `ASFLAGS` vs `SFLAGS` based on the `flag_variables` definition above.
::: python/mozbuild/mozbuild/frontend/emitter.py
(Diff revision 2)
> passthru.variables['AS'] = yasm
> passthru.variables['AS_DASH_C_FLAG'] = ''
> computed_as_flags.resolve_flags('OS',
> context.config.substs.get('YASM_ASFLAGS', []))
>
> -
Stray line deletion.
Attachment #8931143 -
Flags: review?(cmanchester) → review+
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8931146 [details]
Bug 1319222 - Add include flags to HOST_*;
https://reviewboard.mozilla.org/r/202234/#review209990
::: config/config.mk:212
(Diff revision 2)
> +HOST_LINK_CFLAGS = $(COMPUTED_HOST_LINK_CFLAGS)
> +HOST_LINK_CXXFLAGS = $(COMPUTED_HOST_LINK_CXXFLAGS)
For `CompileFlags` we have `C{XX}_LDFLAGS` for the ldflags we also pass to the linked. I guess I'll say let's keep that naming convention, although it's not a perfect fit because for whatever reason we pass includes to link `HOST_SIMPLE_PROGRAMS`. Let's just add a comment or something to the commit message acknowledging the difference between the `HOST_SIMPLE_PROGRAM` and `HOST_PROGRAM` command lines.
Attachment #8931146 -
Flags: review?(cmanchester) → review+
Assignee | ||
Comment 35•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931143 [details]
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;
https://reviewboard.mozilla.org/r/202228/#review209980
> Stray line deletion.
Whoops, fixed!
Assignee | ||
Comment 36•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931146 [details]
Bug 1319222 - Add include flags to HOST_*;
https://reviewboard.mozilla.org/r/202234/#review209990
> For `CompileFlags` we have `C{XX}_LDFLAGS` for the ldflags we also pass to the linked. I guess I'll say let's keep that naming convention, although it's not a perfect fit because for whatever reason we pass includes to link `HOST_SIMPLE_PROGRAMS`. Let's just add a comment or something to the commit message acknowledging the difference between the `HOST_SIMPLE_PROGRAM` and `HOST_PROGRAM` command lines.
Ok, the name is updated.
As for the differences in flag usage between HOST_SIMPLE_PROGRAM and HOST_PROGRAM, it doesn't look like we need INCLUDES or DEFINES in the linker rule for HOST_SIMPLE_PROGRAM anymore (it's only used in layout/style/test for host_ListCSSProperties, and there is a separate compiler step anyway). How about just changing the HOST_SIMPLE_PROGRAM flags to use HOST_CXX_LDFLAGS instead of HOST_CXXFLAGS?
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(cmanchester)
Comment 37•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931146 [details]
Bug 1319222 - Add include flags to HOST_*;
https://reviewboard.mozilla.org/r/202234/#review209990
> Ok, the name is updated.
>
> As for the differences in flag usage between HOST_SIMPLE_PROGRAM and HOST_PROGRAM, it doesn't look like we need INCLUDES or DEFINES in the linker rule for HOST_SIMPLE_PROGRAM anymore (it's only used in layout/style/test for host_ListCSSProperties, and there is a separate compiler step anyway). How about just changing the HOST_SIMPLE_PROGRAM flags to use HOST_CXX_LDFLAGS instead of HOST_CXXFLAGS?
By all means, if the difference isn't needed anymore let's get rid of it! Let's add this as a "pre" patch.
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 49•8 years ago
|
||
mozreview-review |
Comment on attachment 8934596 [details]
Bug 1319222 - Remove INCLUDES from HOST_SIMPLE_PROGRAMS linking;
https://reviewboard.mozilla.org/r/205482/#review211140
Attachment #8934596 -
Flags: review?(cmanchester) → review+
Comment 50•8 years ago
|
||
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05b9eb8ae66b
Remove srcdir from BackendTupfile; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/1c5d8c7e5ddd
Use relobjdir instead of relativedir to determine outputs; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/598cfad59614
Change --param flag to be multiple arguments; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/8e89b51b7f40
Add an SFLAGS ComputedFlags variable for compiling *.S; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/a35541618d92
Compile the generated IPDL and WebIDL sources in the tup backend; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/d115dc76ef73
Remove INCLUDES from HOST_SIMPLE_PROGRAMS linking; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/8b0115b2fed8
Add include flags to HOST_*; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/b6d367955ce6
Enable host compilation in the tup backend; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/7f12adb5e683
Support compiling *.s with yasm; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/a3160ab7a6bf
Enable compilation on all directories in the tup backend; r=chmanchester
Comment 51•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05b9eb8ae66b
https://hg.mozilla.org/mozilla-central/rev/1c5d8c7e5ddd
https://hg.mozilla.org/mozilla-central/rev/598cfad59614
https://hg.mozilla.org/mozilla-central/rev/8e89b51b7f40
https://hg.mozilla.org/mozilla-central/rev/a35541618d92
https://hg.mozilla.org/mozilla-central/rev/d115dc76ef73
https://hg.mozilla.org/mozilla-central/rev/8b0115b2fed8
https://hg.mozilla.org/mozilla-central/rev/b6d367955ce6
https://hg.mozilla.org/mozilla-central/rev/7f12adb5e683
https://hg.mozilla.org/mozilla-central/rev/a3160ab7a6bf
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•