Closed Bug 1266875 Opened 5 years ago Closed 5 years ago

Remove custom install rules from Makefiles

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mshal, Assigned: mshal)

Details

Attachments

(1 file)

We have a bunch of custom install rules in Makefiles that can be moved pretty easily into FINAL_TARGET_FILES and related variables.
Attachment #8744510 - Flags: review?(cmanchester) → review+
Comment on attachment 8744510 [details]
MozReview Request: Bug 1266875 - Remove custom install rules; r?chmanchester

https://reviewboard.mozilla.org/r/48599/#review45331

Nits, nothing worth blocking on.

::: layout/tools/reftest/Makefile.in:10
(Diff revision 1)
>  # We're installing to _tests/reftest
>  TARGET_DEPTH = ../..

This comment makes me think this can go away now, but I don't quite get what effect `TARGET_DEPTH` is having at all.

::: layout/tools/reftest/Makefile.in:17
(Diff revision 1)
> -$(_HARNESS_FILES): $(_DEST_DIR)
> -
>  # copy harness and the reftest extension bits to $(_DEST_DIR)
>  # This needs to happen after jar.mn handling from rules.mk included above.
>  # The order of the :: rules ensures that.
>  libs:: $(_HARNESS_FILES) $(addprefix $(_DEST_DIR)/,$(_HARNESS_PP_FILES))

This reference to `_HARNESS_FILES` can go away. As long as you're here, `_HARNESS_PP_FILES` doesn't seem to be anything anymore.

::: xpcom/tests/moz.build:81
(Diff revision 1)
>  
>  if CONFIG['MOZ_MEMORY']:
>      GeckoCppUnitTests([
>          'TestJemalloc',
>      ])
> +    simple_programs.append('TestJemalloc')

This extra bookeeping is kind of a pain, and I think this all means we (already) have multiple copies of all these in the objdir. That's too bad, but oh well.
https://reviewboard.mozilla.org/r/48599/#review45331

> This comment makes me think this can go away now, but I don't quite get what effect `TARGET_DEPTH` is having at all.

It's used in automation-build.mk, which is used by automation.py, which AFAIK is still used by the Android harnesses. :-/
(In reply to Chris Manchester (:chmanchester) from comment #2)
> ::: layout/tools/reftest/Makefile.in:10
> (Diff revision 1)
> >  # We're installing to _tests/reftest
> >  TARGET_DEPTH = ../..
> 
> This comment makes me think this can go away now, but I don't quite get what
> effect `TARGET_DEPTH` is having at all.

As ted mentions, this has to stick around for now. Though I'd like to port automation-build.mk in another bug as well.

> ::: layout/tools/reftest/Makefile.in:17
> (Diff revision 1)
> > -$(_HARNESS_FILES): $(_DEST_DIR)
> > -
> >  # copy harness and the reftest extension bits to $(_DEST_DIR)
> >  # This needs to happen after jar.mn handling from rules.mk included above.
> >  # The order of the :: rules ensures that.
> >  libs:: $(_HARNESS_FILES) $(addprefix $(_DEST_DIR)/,$(_HARNESS_PP_FILES))
> 
> This reference to `_HARNESS_FILES` can go away. As long as you're here,
> `_HARNESS_PP_FILES` doesn't seem to be anything anymore.

Good catch. Fixed!

> ::: xpcom/tests/moz.build:81
> (Diff revision 1)
> >  
> >  if CONFIG['MOZ_MEMORY']:
> >      GeckoCppUnitTests([
> >          'TestJemalloc',
> >      ])
> > +    simple_programs.append('TestJemalloc')
> 
> This extra bookeeping is kind of a pain, and I think this all means we
> (already) have multiple copies of all these in the objdir. That's too bad,
> but oh well.

Actually the templates add to SIMPLE_PROGRAMS and CPP_UNIT_TESTS internally, so I think we can just use those variables instead of the duplicate bookkeeping. New patch upcoming...
Comment on attachment 8744510 [details]
MozReview Request: Bug 1266875 - Remove custom install rules; r?chmanchester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48599/diff/1-2/
Comment on attachment 8744510 [details]
MozReview Request: Bug 1266875 - Remove custom install rules; r?chmanchester

Re-review on xpcom/tests/ changes.
Attachment #8744510 - Flags: review+ → review?(cmanchester)
Comment on attachment 8744510 [details]
MozReview Request: Bug 1266875 - Remove custom install rules; r?chmanchester

https://reviewboard.mozilla.org/r/48599/#review45905
Attachment #8744510 - Flags: review?(cmanchester) → review+
Backed out for pgo build bustage in xpcshell/xpcom/tests/unit/nsIFileEnumerator.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/69f8521535b0
Job with pgo runs which failed: https://hg.mozilla.org/integration/mozilla-inbound/rev/69f8521535b0
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=26725210&repo=mozilla-inbound

08:50:54     INFO -  gmake[6]: Entering directory `/builds/slave/m-in-l64-pgo-00000000000000000/build/src/obj-firefox/xpcom/tests'
08:50:54     INFO -  gmake[6]: *** No rule to make target `nsIFileEnumerator', needed by `../../_tests/xpcshell/xpcom/tests/unit/nsIFileEnumerator'.  Stop.
08:50:54     INFO -  gmake[6]: Leaving directory `/builds/slave/m-in-l64-pgo-00000000000000000/build/src/obj-firefox/xpcom/tests'
08:50:54     INFO -  gmake[5]: *** [xpcom/tests/misc] Error 2
Flags: needinfo?(mshal)
Doh. It seems rules.mk decides to skip SIMPLE_PROGRAMS during the MOZ_PROFILE_GENERATE phase (bug 602245). glandium, do you have a suggestion for how to handle this in moz.build? I don't see anywhere that we currently change behavior based on MOZ_PROFILE_GENERATE / MOZ_PROFILE_USE. The only PGO changes we make are to disable it for certain files or directories.
Flags: needinfo?(mshal) → needinfo?(mh+mozilla)
This may or may not be a lot of work, but why not try to get rid of the maybe_clobber_profiledbuild step of PGO builds?
Flags: needinfo?(mh+mozilla)
I filed bug 1268658 as a followup. I'll land the parts that aren't installing SIMPLE_PROGRAMS for now.
https://hg.mozilla.org/mozilla-central/rev/08b80f847779
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.