Closed
Bug 921003
Opened 11 years ago
Closed 11 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)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla27
People
(Reporter: glandium, Assigned: glandium)
References
(Blocks 1 open bug)
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.
Assignee | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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?
Assignee | ||
Comment 3•11 years ago
|
||
Refreshed after bug 921070
Attachment #810861 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #810550 -
Attachment is obsolete: true
Attachment #810550 -
Flags: review?(gps)
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #810861 -
Attachment is obsolete: true
Attachment #810861 -
Flags: review?(gps)
Assignee | ||
Comment 5•11 years ago
|
||
(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].
Assignee | ||
Comment 6•11 years ago
|
||
This one, however, is different, it fixes another typo.
Attachment #810889 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #810883 -
Attachment is obsolete: true
Attachment #810883 -
Flags: review?(gps)
Assignee | ||
Comment 7•11 years ago
|
||
Sorry for the flood. Added debugging messages giving the number of skipped directories for each tier.
Attachment #810902 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #810889 -
Attachment is obsolete: true
Attachment #810889 -
Flags: review?(gps)
Comment 8•11 years ago
|
||
What about jar.mn files?
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #810902 -
Attachment is obsolete: true
Comment 12•11 years ago
|
||
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+
Comment 13•11 years ago
|
||
pgomerge only runs in directories with LIBRARY or PROGRAM. I can't fathom how we'd hit it and not have a Makefile present.
Assignee | ||
Comment 14•11 years ago
|
||
(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.
Assignee | ||
Comment 15•11 years ago
|
||
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.
Assignee | ||
Comment 16•11 years ago
|
||
Make opt-in, as i originally intended.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c4ac1d21d29
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c5906eed61fc
https://hg.mozilla.org/mozilla-central/rev/1c4ac1d21d29
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•