837803 - Proper dependencies for FHR's preprocessed files

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

Use of the preprocessor to concatenate JSMs doesn't honour proper dependencies.

In this bug, I'll provide a quick hacky solution until bug 837792 is in place or until we unhack JSMs in bug 836285.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: Add PREREQS option to PP_TARGETS, v1

I think we had talked about this on IRC many months ago. Patch is trivial. Whitespace is from converting tabs to spaces.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 2: Capture FHR dependencies properly, v1

Patch should be self-explanatory.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 709809 [details] [diff] [review]
Part 1: Add PREREQS option to PP_TARGETS, v1

Review of attachment 709809 [details] [diff] [review]:
-----------------------------------------------------------------

I'm wondering. Do we really need that, when you can add the dependencies in the Makefile.in as a temporary solution until bug 837792. I mean, is that going to be useful for something else?

Comment 4

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #3)
> I'm wondering. Do we really need that, when you can add the dependencies in
> the Makefile.in as a temporary solution until bug 837792. I mean, is that
> going to be useful for something else?

I could create one-off make rules, sure.

I figured the generic solution in rules.mk might be used by someone if e.g. they want to force the preprocessor to run after some other target. I believe the solution in this bug is also the easiest to implement :)

Comment 5

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > I'm wondering. Do we really need that, when you can add the dependencies in
> > the Makefile.in as a temporary solution until bug 837792. I mean, is that
> > going to be useful for something else?
> 
> I could create one-off make rules, sure.
> 
> I figured the generic solution in rules.mk might be used by someone if e.g.
> they want to force the preprocessor to run after some other target. I
> believe the solution in this bug is also the easiest to implement :)

$(FINAL_TARGET)/modules/HealthReport.jsm: ../common/async.js ../common/bagheeraclient.js etc.

is not very much different.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Explicit rules in Makefile.in, v1

Alternate patch to parts 1 and 2.

Comment 7

[Richard Newman [:rnewman]]

Personally I find the first approach to be clearer.

I know we want to eventually eliminate this concatenation, but I generally favor increasing (documented! efficient!) expressiveness in our toolchain over time rather than accruing special-case workarounds, and it's not entirely clear *when* we'll get "free" compartments and thus be able to eliminate the concatenation.

Note that we've taken janky approaches to working around this very same in the past: see 

  http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#1232

This is definitely a real use case.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 709863 [details] [diff] [review]
Explicit rules in Makefile.in, v1

Review of attachment 709863 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/Makefile.in
@@ +48,5 @@
>  include $(topsrcdir)/config/rules.mk
> +
> +$(FINAL_TARGET)/modules/HealthReport.jsm: $(healthreport_depends)
> +	$(RM) $@
> +	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) $(XULPPFLAGS) $< > $@

You don't need to do that. Just keep using PP_TARGETS, and define the dependency only (which needs to be after including rules.mk because FINAL_TARGET is defined in config.mk, iirc)

Comment 9

[Mike Hommey [:glandium]]

(In reply to Richard Newman [:rnewman] from comment #7)
> Personally I find the first approach to be clearer.
> 
> I know we want to eventually eliminate this concatenation, but I generally
> favor increasing (documented! efficient!) expressiveness in our toolchain
> over time rather than accruing special-case workarounds, and it's not
> entirely clear *when* we'll get "free" compartments and thus be able to
> eliminate the concatenation.

The fix for the present case is actually bug 837792, and it will have to happen because it's not only a problem for js files.


> Note that we've taken janky approaches to working around this very same in
> the past: see 
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.
> in#1232
> 
> This is definitely a real use case.

Which will be addressed in bug 837792. We don't need a temporary expressive way to fix the issue.

Comment 10

[Gregory Szorc [:gps]]

Attachment: List extra prereqs in Makefile.in, v1

I forgot you could add make rules without recipes to define extra prerequisites. Oh, make.

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/2888189660d5

Comment 12

[Richard Newman [:rnewman]]

(In reply to Mike Hommey [:glandium] from comment #9)

> Which will be addressed in bug 837792. We don't need a temporary expressive
> way to fix the issue.

Great, glad to hear it. Sorry I didn't see Comment 3!

Comment 13

[Gregory Szorc [:gps]]

Attachment: Part 2: Capture dependencies for DataReportingService.js, v1

Missed a file.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/54686023b7f1

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/2888189660d5
https://hg.mozilla.org/mozilla-central/rev/54686023b7f1