921003 - For a given tier, skip directories without a Makefile and without variables in moz.build that are relevant to that tier

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

With pseudo derecurse, all directories are independent in the directory traversal, so skipping a directory doesn't skip all its children.

There are many directories in the tree that have almost nothing interesting in their moz.build (like just having DIRS or some other minimal stuff), and it's a waste of time to go through those directories for all tiers when they may just need to do something for one or two of the tiers.

Comment 1

[Mike Hommey [:glandium]]

Attachment: For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

This skips 395 directories for libs, 277 for export and 434 for tools. This depends on my annotations in VARIABLES being right.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 810550 [details] [diff] [review]
For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

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

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +60,3 @@
>  #
> +# Tier says for which specific tier the variable has an effect.
> +# Valid tiers are:

How about None?

Comment 3

[Mike Hommey [:glandium]]

Attachment: For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

Refreshed after bug 921070

Comment 4

[Mike Hommey [:glandium]]

Attachment: For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

Adjusted to part 2 move, and added a couple guards against filling things up for the binaries target when pseudo derecurse is off.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Adjusted to part 2 move, and added a couple guards against filling things up
> for the binaries target when pseudo derecurse is off.

Ignore this comment, this was meant for part 3 of bug 905973. This new attachment is the same patch as attachment 810861 [details] [diff] [review].

Comment 6

[Mike Hommey [:glandium]]

Attachment: For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

This one, however, is different, it fixes another typo.

Comment 7

[Mike Hommey [:glandium]]

Attachment: For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

Sorry for the flood. Added debugging messages giving the number of skipped directories for each tier.

Comment 8

[[e-mail]]

What about jar.mn files?

Comment 9

[Mike Hommey [:glandium]]

(In reply to [e-mail] from comment #8)
> What about jar.mn files?

mmm good point.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 810902 [details] [diff] [review]
For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

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

There's still an export:: rule in rules.mk for pgomerge.py. This is tracked in bug 892693. There's got to be a directory somewhere without a Makefile that still gets this rule hit. It could probably be removed easily enough, possibly through a custom Python script that just does an os.walk or via something that takes moz.build-derived file lists.

Theoretically, sandbox.py is meant to be a standalone and generic sandboxing implementation. I'm pretty sure we deviate from this today. So your addition of Mozilla-centric functionality can be cleaned up later if we ever want to do that. Just keep it in mind for next time. That being said, moving get_affected_rules to reader.py shouldn't be too difficult...

This patch uses "rule" a lot of places where I think you should be using "target" or "tier" instead.

Anyway, I like what I see here. Not granting r+ because there's a few too many nits for my liking and because I'm jetlagged and don't want to give r+ out of principle.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +312,5 @@
>          derecurse = self.environment.substs.get('MOZ_PSEUDO_DERECURSE', '').split(',')
>          self._parallel_export = False
> +        self._no_skip = False
> +        if derecurse != ['']:
> +            if not 'no-parallel-export' in derecurse:

Nit: Use the 'not in' operator instead of relying on operator precedence (I should have caught this before).

@@ +315,5 @@
> +        if derecurse != ['']:
> +            if not 'no-parallel-export' in derecurse:
> +                self._parallel_export = True
> +            if 'no-skip' in derecurse:
> +                self._no_skip = True

self._no_skip = 'no-skip' in derecurse

@@ +421,5 @@
>          """
> +        if not self._no_skip:
> +            for tier, skip in self._may_skip.items():
> +                self.log(logging.DEBUG, 'fill_root_mk',
> +                    { 'number': len(skip), 'tier': tier },

Nit: cuddle braces

@@ +827,5 @@
> +
> +        affected_rules = set(obj.affected_rules)
> +        if obj.is_tool_dir and 'libs' in affected_rules:
> +            affected_rules.remove('libs')
> +            affected_rules.add('tools')

This seems like something that should be performed farther up the stack, not in a backend. Although, directory traversal is make specific, so meh.

Comment 11

[Mike Hommey [:glandium]]

Attachment: For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

This addresses the review comments and the jar.mn issue. It also fixes an issue i found with the previous patch that would skip directories it shouldn't skip. It however doesn't do anything about pgomerge. I want to force disable pseudo derecurse on pgo anyways (bug 922520)

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 812494 [details] [diff] [review]
For a given tier, skip directories without a Makefile.in and without variables in moz.build that are relevant to that tier

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

This patch is kinda crazy. But if it works, it should be a huge win until we can write out decent backend files to avoid the recursion overhead. I approve.

Can I convince you to write some minimal tests for the skip behavior?

Comment 13

[(not currently active) Ted Mielczarek]

pgomerge only runs in directories with LIBRARY or PROGRAM. I can't fathom how we'd hit it and not have a Makefile present.

Comment 14

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> pgomerge only runs in directories with LIBRARY or PROGRAM. I can't fathom
> how we'd hit it and not have a Makefile present.

In the (near) future, that will be true.

Comment 15

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5906eed61fc

(In reply to Gregory Szorc [:gps] from comment #12)
> Can I convince you to write some minimal tests for the skip behavior?

I'm not sure how to write a test for that.

Comment 16

[Mike Hommey [:glandium]]

Make opt-in, as i originally intended.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c4ac1d21d29

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c5906eed61fc
https://hg.mozilla.org/mozilla-central/rev/1c4ac1d21d29