Closed Bug 1241022 Opened 4 years ago Closed 4 years ago

Rework affected tiers for the recursive make backend

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: glandium, Assigned: glandium)

Details

Attachments

(11 files)

4.62 KB, patch
gps
: review+
Details | Diff | Splinter Review
5.16 KB, patch
gps
: review+
Details | Diff | Splinter Review
1.82 KB, patch
gps
: review+
Details | Diff | Splinter Review
3.49 KB, patch
gps
: review+
Details | Diff | Splinter Review
6.34 KB, patch
gps
: review+
Details | Diff | Splinter Review
2.24 KB, patch
gps
: review+
Details | Diff | Splinter Review
4.12 KB, patch
gps
: review+
Details | Diff | Splinter Review
3.69 KB, patch
gps
: review+
Details | Diff | Splinter Review
2.00 KB, patch
gps
: review+
Details | Diff | Splinter Review
3.46 KB, patch
gps
: review+
Details | Diff | Splinter Review
44.77 KB, patch
gps
: review+
Details | Diff | Splinter Review
The model we use for affected tiers, started in bug 921003 works less and less properly. Some of them are now blatantly wrong. Some others are subtly wrong.

Plus, they only really apply to the recursive make backend, while kind of defined globally.
As an example, we are currently doing `make -C $dir export` in 503 directories (grepping "Entering" in a `mach build export -v` log for a local linux build). 315 or those end with "Nothing to be done for 'export'".

Assuming I didn't mess things up, I got that down to 287 `make -C $dir export` and 97 "Nothing to be done for 'export'" (we still have those because we still assume that we can't skip a directory with a Makefile.in in it).
Historically, all tiers were handled as opt-out (may_skip), until we
added the first opt-in tier (no_skip). It doesn't make much sense to
differentiate them anymore, so handle them all as opt-in.
This initiates a move off affected_tiers in VARIABLES definition to
explicit in-backend handling, which will hopfully make things clearer.

HAS_MISC_RULE is currently used to opt-in to the misc tier in a few
directories with a misc:: rule.  It is, in fact, mostly used for custom
xpi creation, which will be separately addressed in bug 1240676, so it
will eventually go away entirely, but in the meantime, we send it as a
throwaway passthru.
The are passthru variables that don't actually affect any tier per the
backend itself. They do affect the `export` tier by way of the Makefile.in
along the moz.build defining them, and the existence of those
Makefile.in already guarantees those directories not to be skipped for
`export`.
Those are non-passthru variables with the same property as
ANDROID_GENERATED_RESFILES, ANDROID_APK_NAME and ANDROID_APK_PACKAGE, in
that they don't affect tiers through the backend code itself, but from
the Makefile.in along the moz.build they are defined in.
GENERATED_FILES impacts the export tier through the config/rules.mk
definitions, now moved to the backend itself, so that everything is
close to each other.
JAR_MANIFESTS affects the libs tiers through config/rules.mk rules.
While we could move the rules in the backend, they are too complex to
just do that now.
They are respectively using PP_TARGETS and INSTALL_TARGETS. Both affect
the misc tier since bug 1240671, per the *_TARGET value they set in the
backend.

This has the nice side effect of now skipping directories where test
harness files are handled by install manifests.
Those are the worst offenders of affected_tiers. The rules to handle
them are all in directories that is not necessarily related to where
the variables are defined, each of which has a Makefile.in for those
rules, which forces export to go through them.
PI_NAME affects no tier on its own, as it merely changes paths where
things end up that are defined by other variables.

FILES_PER_UNIFIED_FILE had an erroneous value.
Attachment #8709838 - Flags: review?(gps)
Attachment #8709839 - Flags: review?(gps)
Attachment #8709840 - Flags: review?(gps)
Attachment #8709841 - Flags: review?(gps)
Attachment #8709842 - Flags: review?(gps)
Attachment #8709843 - Flags: review?(gps)
Attachment #8709844 - Flags: review?(gps)
Attachment #8709845 - Flags: review?(gps)
Attachment #8709846 - Flags: review?(gps)
Attachment #8709847 - Flags: review?(gps)
Attachment #8709848 - Flags: review?(gps)
Comment on attachment 8709838 [details] [diff] [review]
Handle all tiers as opt-in in the recursive make backend

Review of attachment 8709838 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +784,5 @@
>                          {'path': makefile}, 'Substituting makefile: {path}')
>                      self._makefile_in_count += 1
>  
> +                    if bf.relobjdir:
> +                        for tier in ('export', 'libs',):

This feels like it needs a comment. But I suspect you'll address this in a subsequent patch.
Attachment #8709838 - Flags: review?(gps) → review+
Attachment #8709839 - Flags: review?(gps) → review+
Attachment #8709840 - Flags: review?(gps) → review+
Attachment #8709841 - Flags: review?(gps) → review+
Attachment #8709842 - Flags: review?(gps) → review+
Attachment #8709843 - Flags: review?(gps) → review+
Attachment #8709844 - Flags: review?(gps) → review+
Attachment #8709845 - Flags: review?(gps) → review+
Attachment #8709846 - Flags: review?(gps) → review+
Attachment #8709847 - Flags: review?(gps) → review+
Comment on attachment 8709848 [details] [diff] [review]
Remove affected_tiers

Review of attachment 8709848 [details] [diff] [review]:
-----------------------------------------------------------------

Nice series.
Attachment #8709848 - Flags: review?(gps) → review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.