Closed Bug 921003 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 5 obsolete files)

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.
This skips 395 directories for libs, 277 for export and 434 for tools. This depends on my annotations in VARIABLES being right.
Attachment #810550 - Flags: review?(gps)
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?
Attachment #810550 - Attachment is obsolete: true
Attachment #810550 - Flags: review?(gps)
Adjusted to part 2 move, and added a couple guards against filling things up for the binaries target when pseudo derecurse is off.
Attachment #810883 - Flags: review?(gps)
Attachment #810861 - Attachment is obsolete: true
Attachment #810861 - Flags: review?(gps)
(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].
This one, however, is different, it fixes another typo.
Attachment #810889 - Flags: review?(gps)
Attachment #810883 - Attachment is obsolete: true
Attachment #810883 - Flags: review?(gps)
Sorry for the flood. Added debugging messages giving the number of skipped directories for each tier.
Attachment #810902 - Flags: review?(gps)
Attachment #810889 - Attachment is obsolete: true
Attachment #810889 - Flags: review?(gps)
Blocks: 921313
What about jar.mn files?
(In reply to neil@parkwaycc.co.uk from comment #8)
> What about jar.mn files?

mmm good point.
Blocks: 892644
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.
Attachment #810902 - Flags: review?(gps) → feedback+
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)
Attachment #812494 - Flags: review?(gps)
Attachment #810902 - Attachment is obsolete: true
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?
Attachment #812494 - Flags: review?(gps) → review+
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 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.
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.
Blocks: 923060
https://hg.mozilla.org/mozilla-central/rev/c5906eed61fc
https://hg.mozilla.org/mozilla-central/rev/1c4ac1d21d29
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: 926733
Blocks: 933145
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.