Closed
Bug 1319222
Opened 7 years ago
Closed 6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 years ago
|
Flags: needinfo?(cmanchester)
Comment 37•6 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•6 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•6 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•6 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: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
•