Closed Bug 1474028 Opened 5 years ago Closed 5 years ago

Add a moz.build driven way to exclude targets from the default build

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1474028 - Add a way to exclude libraries from the default build.
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1474028 - Use output categories to exclude the gtest libxul from the default tup build.
59 bytes, text/x-review-board-request
Details 
This came up dealing with the gtest libxul situation in bug 1468547. I have a patch that will let up label a library from moz.build to exclude it from the default build and put it and its dependencies behind their own target. The changes to the make backend were a little wonky so I want to test a few more things before requesting review.
I'm not thrilled about some of the subtleties of adding this to the make backend, but what I have so far here works well enough, I'll post for review shortly.
Attachment #8992115 - Flags: review?(core-build-config-reviews) → review?(ted)
Comment on attachment 8992116 [details]
Bug 1474028 - Use output categories to exclude the gtest libxul from the default tup build.

https://reviewboard.mozilla.org/r/256372/#review264830
Attachment #8992116 - Flags: review+
Attachment #8992116 - Flags: review?(core-build-config-reviews)
Comment on attachment 8992115 [details]
Bug 1474028 - Add a way to exclude libraries from the default build.

https://reviewboard.mozilla.org/r/256370/#review267500

The compile graph traversal bits are definitely a bit complicated, but I think it's worthwhile for the cleanup you're able to do elsewhere. Nice work!

::: config/rules.mk:449
(Diff revision 1)
>  
>  compile:: host target
>  
>  host:: $(HOST_LIBRARY) $(HOST_PROGRAM) $(HOST_SIMPLE_PROGRAMS) $(HOST_RUST_PROGRAMS) $(HOST_RUST_LIBRARY_FILE) $(HOST_SHARED_LIBRARY)
>  
> -target:: $(LIBRARY) $(SHARED_LIBRARY) $(PROGRAM) $(SIMPLE_PROGRAMS) $(RUST_LIBRARY_FILE) $(RUST_PROGRAMS)
> +target:: $(filter-out $(MOZBUILD_NON_DEFAULT_TARGETS),$(LIBRARY) $(SHARED_LIBRARY) $(PROGRAM) $(SIMPLE_PROGRAMS) $(RUST_LIBRARY_FILE) $(RUST_PROGRAMS))

As a follow-up, maybe we could move these bits out of rules.mk and have them emitted into backend.mk instead?

::: python/mozbuild/mozbuild/backend/recursivemake.py:830
(Diff revision 1)
> +        for root in compile_roots:
> +            # If this is a non-default target, separate the root from the
> +            # rest of the compile graph.
> +            target_name = mozpath.basename(root)
> +
> +            if target_name not in ('target', 'host'):

Feels like this could stand to be a bit more structured than just a string comparison, but it's fine for now.

::: python/mozbuild/mozbuild/backend/recursivemake.py:1294
(Diff revision 1)
>                                                          ' '.join(make_quote(shell_quote(f)) for f in flags)))
>  
> +    def _process_non_default_target(self, libdef, target_name, backend_file):
> +        fragment = Makefile()
> +        rule = fragment.create_rule(targets=['%s:' % libdef.output_category])
> +        rule.add_dependencies([target_name])

Does using the Makefile type here buy you much? Everything else just seems to write Makefile syntax directly to `backend_file`.

::: python/mozbuild/mozbuild/frontend/context.py:1560
(Diff revision 1)
>          differ from the library code name.
>  
>          Implies FORCE_SHARED_LIB.
>          """),
>  
> +    'SHARED_LIBRARY_OUTPUT_CATEGORY': (unicode, unicode,

I kinda feel like it might be time to give the moz.build context objects more structure instead of tying together sets of disjoint variables in the emitter...

::: toolkit/library/gtest/Makefile.in
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -# Enforce that the clean/distclean rules removes everything that needs

\o/

::: toolkit/library/gtest/Makefile.in:5
(Diff revision 1)
> -# Not including backend.mk makes traversing this directory do nothing.
> -STANDALONE_MAKEFILE = 1
> -
> -else
> -
>  include $(topsrcdir)/toolkit/library/libxul.mk

Oh wow, we could totally get rid of libxul.mk (and thus this entire Makefile.in):
https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/toolkit/library/libxul.mk
Attachment #8992115 - Flags: review?(ted) → review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39a528147c34
Add a way to exclude libraries from the default build. r=ted
https://hg.mozilla.org/integration/autoland/rev/52bd814d3589
Use output categories to exclude the gtest libxul from the default tup build. r=mshal
https://hg.mozilla.org/mozilla-central/rev/39a528147c34
https://hg.mozilla.org/mozilla-central/rev/52bd814d3589
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1482690
Backed out in https://hg.mozilla.org/mozilla-central/rev/a4cff96e8c08ef5b7e07f68cee37b94feca49c90 for breaking symbols. The logic there to skip dumping symbols in non-default directories is almost entirely wrong.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f3a2d3db15e6
Add a way to exclude libraries from the default build. r=ted
https://hg.mozilla.org/integration/autoland/rev/37cf2f43323b
Use output categories to exclude the gtest libxul from the default tup build. r=mshal
https://hg.mozilla.org/mozilla-central/rev/f3a2d3db15e6
https://hg.mozilla.org/mozilla-central/rev/37cf2f43323b
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.