Closed Bug 1098135 Opened 7 years ago Closed 7 years ago

Traverse directories with misc:: Makefile.in targets

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attached file MozReview Request: bz://1098135/gps (obsolete) —
Attachment #8521995 - Flags: review?(mh+mozilla)
/r/579 - Bug 1098135 - Traverse directories with misc:: targets

Pull down this commit:

hg pull review -r 658c882fe976f8567881e2f8ddedb74cf5e0b1b9
https://reviewboard.mozilla.org/r/577/#review233

::: modules/libpref/Makefile.in
(Diff revision 1)
>  	$(INSTALL) $^ $(DIST)/bin/

This strikes me as the exact case where we shouldn't be doing a simple s/libs/misc/, and transform that crap to use PP_TARGETS instead.

::: browser/experiments/Makefile.in
(Diff revision 1)
>  	$(NSINSTALL) -D $(testdir); \

Replace this NSINSTALL -D with a mkdir_deps while you're here.

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 1)
> +                    if b'misc' in content and re.search('(?:^|\s)misc.*::', content, re.M):

You need the objdir == self.environment.topobjdir test.

Come to think of it, how about making that actually explicit by requiring a declaration in moz.build?
/r/579 - Bug 1098135 - Traverse directories with misc rules

Pull down this commit:

hg pull review -r 97d3587d1a573ec06d700f95a9a1f37b4995796b
https://reviewboard.mozilla.org/r/577/#review277

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 2)
> +            affected.add('misc')

The fact that HAS_MISC_RULE is associated to misc in VARIABLES should be enough. If it's not there's something wrong.

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 2)
> +        Many rules still existing in Makefile.in files. We highly prefer that

s/existing/exist/. Please also add a note that while that's what we would like, it's not necessarily easy because of implicit dependencies, and that any conversion should be done very carefully.

BTW, please split the frontend logic and the partial conversion in two patches.

::: dom/workers/test/extensions/traditional/Makefile.in
(Diff revision 2)
> -libs::
> +misc::

Please add a comment that this relies on xpi-stage/$(XPI_NAME) being entirely filled by export and compile, as well as misc rules occuring before this one (but not after, which there aren't, fortunately).

::: testing/specialpowers/Makefile.in
(Diff revision 2)
> -libs-preqs = \
> +misc:: $(call mkdir_deps,$(TEST_EXTENSIONS_DIR))

And here is exactly why the disclaimer is needed: this can't work because there is a jar.mn and jar manifests are still processed in libs.
/r/579 - Bug 1098135 - Traverse directories with misc rules
/r/621 - Bug 1098135 - Convert some rules to misc tier

Pull down these commits:

hg pull review -r 666cb72fe2741331b554bfd826411f7e5fe4036f
https://reviewboard.mozilla.org/r/579/#review279

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 3)
> +        Please note that converting rules to the ``misc`` tier must be done

converting ``libs`` rules

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 3)
> +        Many rules still exist in Makefile.in files. We highly prefer that

Many ``libs`` rules (because we also have e.g. export rules)
https://reviewboard.mozilla.org/r/621/#review281

::: dom/workers/test/extensions/traditional/Makefile.in
(Diff revision 1)
> -libs::
> +misc::

Please add a comment as to why this is safe (cf. previous review)
Attachment #8521995 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/dad976bc94f3
https://hg.mozilla.org/mozilla-central/rev/b1a9e41d3f4b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1099232
Attachment #8521995 - Attachment is obsolete: true
Attachment #8618620 - Flags: review+
Attachment #8618621 - Flags: review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.