1266875 - Remove custom install rules from Makefiles

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

We have a bunch of custom install rules in Makefiles that can be moved pretty easily into FINAL_TARGET_FILES and related variables.

Comment 1

[Michael Shal [:mshal]]

Attachment: MozReview Request: Bug 1266875 - Remove custom install rules; r?chmanchester

Review commit: https://reviewboard.mozilla.org/r/48599/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48599/

Comment 2

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 3

[(not currently active) Ted Mielczarek]

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. :-/

Comment 4

[Michael Shal [:mshal]]

(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 5

[Michael Shal [:mshal]]

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 6

[Michael Shal [:mshal]]

Comment on attachment 8744510 [details]
MozReview Request: Bug 1266875 - Remove custom install rules; r?chmanchester

Re-review on xpcom/tests/ changes.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8744510 [details]
MozReview Request: Bug 1266875 - Remove custom install rules; r?chmanchester

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

Comment 8

[Pulsebot]

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

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

[Michael Shal [: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.

Comment 11

[Mike Hommey [:glandium]]

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?

Comment 12

[Michael Shal [:mshal]]

I filed bug 1268658 as a followup. I'll land the parts that aren't installing SIMPLE_PROGRAMS for now.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08b80f847779

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08b80f847779