Closed Bug 1209458 Opened 4 years ago Closed 4 years ago

Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: gfritzsche, Assigned: yarik.sheptykin, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [unifiedTelemetry] [measurement:client] [lang=js])

Attachments

(2 files)

(In reply to Philip Chee from bug 1206085, comment #24)
> You forgot to update the moz.build file
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> moz.build?rev=c646a88160d7#44
> Move from EXTRA_PP_JS_MODULES to EXTRA_JS_MODULES
> 
> In TelemetrySession.jsm you can remove the final bit of preprocessing using
> AppConstants.jsm
> 
> In toolkit/modules/moz.build add HISTOGRAMS_FILE_VERSION to the section that
> begins (This may not be needed):
> 
> for var in ('ANDROID_PACKAGE_NAME',
>             .......
> 
> Then in AppConstants.jsm near the end of the file add a line like:
> 
>   HISTOGRAMS_FILE_VERSION: @HISTOGRAMS_FILE_VERSION@
> 
> Then in TelemetrySession.jsm:
> 
> -      revision: HISTOGRAMS_FILE_VERSION,
> +      revision: AppConstants.HISTOGRAMS_FILE_VERSION,
(In reply to Georg Fritzsche [:gfritzsche] from bug 1206085, comment #28)
> https://reviewboard.mozilla.org/r/19939/#review18551
> 
> While we are doing this, we should also address/move the
> `HISTOGRAM_FILE_VERSION` definition:
> https://dxr.mozilla.org/mozilla-central/rev/
> 031db40e2b558c7e4dd0b4c565db4a992c1627c8/toolkit/components/telemetry/
> Makefile.in#9
> 
> All this really has is the current revision of the repo (e.g.
> "https://hg.mozilla.org/mozilla-central/rev/6256ec9113c1"), so lets make
> this just a more general constant on AppConstants.jsm like `REVISION` or
> `SOURCE_REVISION`.
> 
> I also wonder if we don't have a cleaner solution in or build system now.
> 
> ::: toolkit/components/telemetry/moz.build:46
> (Diff revision 4)
> > -    'TelemetryController.jsm',
> > +  'TelemetryController.jsm',
> > -    'TelemetryEnvironment.jsm',
> > +  'TelemetryEnvironment.jsm',
> > -    'TelemetrySession.jsm',
> > +  'TelemetrySession.jsm',
> 
> These are not testing-specific modules, they belong into `EXTRA_JS_MODULES`.
Assignee: nobody → yarik.sheptykin
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> (In reply to Georg Fritzsche [:gfritzsche] from bug 1206085, comment #28)
> > https://reviewboard.mozilla.org/r/19939/#review18551
> > 
> > While we are doing this, we should also address/move the
> > `HISTOGRAM_FILE_VERSION` definition:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 031db40e2b558c7e4dd0b4c565db4a992c1627c8/toolkit/components/telemetry/
> > Makefile.in#9
> > 
> > All this really has is the current revision of the repo (e.g.
> > "https://hg.mozilla.org/mozilla-central/rev/6256ec9113c1"), so lets make
> > this just a more general constant on AppConstants.jsm like `REVISION` or
> > `SOURCE_REVISION`.
> > 
> > I also wonder if we don't have a cleaner solution in or build system now.

gps, do you know if we have something cleaner by now?
All we really need is to have a string of the current source revision (e.g. "https://hg.mozilla.org/mozilla-central/rev/6256ec9113c1") in a JS module.
Do we still need that Makefile.in hack or do we have something nicer available now?
Flags: needinfo?(gps)
Status: NEW → ASSIGNED
Source revision information currently requires make foo. See config/makefiles/rcs.mk and various references in Makefile.in files. The easy solution is to add some one-off defines to a backend.mk-defined variable in a Makefile.in file so the source revision is populated at make/preprocessor time.
Flags: needinfo?(gps)
Hi Georg! Hi Phillip!

First, when I write SOURCE_REVISION I mean HISTOGRAMS_FILE_VERSION. I renamed it meanwhile according to Georg`s suggestion in comment 1.

I am stuck trying to get SOURCE_REVISION from Makefile.in into toolkit/modules/moz.build. I followed the instructions in the description but the configuration of the build system fails when I:

> > In toolkit/modules/moz.build add SOURCE_REVISION to the section that
> > begins (This may not be needed):
> > 
> > for var in ('ANDROID_PACKAGE_NAME',
> >             .......

i.e. = DEFINES['SOURCE_REVISION'] = CONFIG['SOURCE_REVISION'] triggers a failure.
Without that configuration succeeds but building AppConstants.jsm fails.

I was expecting that the moz-build system would magically detect the SOURCE_REVISION and add it to the CONFIG. This does not seem to be the case, so we should make configure aware of SOURCE_REVISION. I looked around to find an example of how others handle this an in all cases I could find people edit configure.in directly. see http://hg.mozilla.org/mozilla-central/rev/eb43469cf706 for ex. Is this the right way? Should I move SOURCE_REVISION declaration into configure.in?
Flags: needinfo?(philip.chee)
Flags: needinfo?(gfritzsche)
Thanks to Teds help i figured out what we need to do here.
I'll put that part up here and leave the Telemetry parts to you Iaroslav :)
(including cleaning up the Makefile.in there etc.)
Flags: needinfo?(philip.chee)
Flags: needinfo?(gfritzsche)
Comment on attachment 8667984 [details] [diff] [review]
Expose the current hg revision URL on AppConstants.jsm.

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

::: configure.in
@@ +8815,5 @@
>  
> +# On official builds, we need to know in Telemetry what revision this is built from.
> +# This is e.g. needed to match incoming data to a specific revision of the Histograms.json
> +# file.
> +if test "$MOZILLA_OFFICIAL" && test -d ${_topsrcdir}/.hg; then

I would leave out the MOZILLA_OFFICIAL bit, doesn't seem harmful to put this in for all builds, since we're checking for .hg already.
Attachment #8667984 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Comment on attachment 8667984 [details] [diff] [review]
> Expose the current hg revision URL on AppConstants.jsm.
> 
> Review of attachment 8667984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +8815,5 @@
> >  
> > +# On official builds, we need to know in Telemetry what revision this is built from.
> > +# This is e.g. needed to match incoming data to a specific revision of the Histograms.json
> > +# file.
> > +if test "$MOZILLA_OFFICIAL" && test -d ${_topsrcdir}/.hg; then
> 
> I would leave out the MOZILLA_OFFICIAL bit, doesn't seem harmful to put this
> in for all builds, since we're checking for .hg already.

I'm not sure if we should have a value there for local-only revisions (applied patches, local commits, ...).
Although we could just make Telemetry only submit this on official builds, maybe thats the better approach.
What do you think?
Flags: needinfo?(ted)
I was mostly thinking that relying on MOZILLA_OFFICIAL makes it slightly more of a pain to test, but I guess it's not really that important. Whatever you think is best is fine.
Flags: needinfo?(ted)
Ok, lets go with the current version then - if we have different needs we can always change this later.
Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r?gfritzsche
Attachment #8669564 - Flags: review?(gfritzsche)
Attachment #8669564 - Flags: review?(gfritzsche) → review+
Comment on attachment 8669564 [details]
MozReview Request: Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r=ted,gfritzsche

https://reviewboard.mozilla.org/r/21237/#review19131

Ok, i would have expected this to be based on my patch.
But its fine with me either way, you can just make this one r=ted,gfritzsche and go ahead.
(In reply to Georg Fritzsche [:gfritzsche] [away Oct 6 - Oct 8] from comment #12)

Thanks for the review, Georg!

> Ok, i would have expected this to be based on my patch.

Sorry about that. I imported your patch and mixed it into my tree. I was not sure how reviewboard deals with such things and expected it to delete all earlier attachments to a bug just like it does when I update a review. So I smashed everything together with histedit and sent it to review.
Just want you to know that there is no evil plan going on, rather a lack of experience. Next time I will try to do it properly.

> But its fine with me either way, you can just make this one r=ted,gfritzsche
> and go ahead.

I pushed this patch to the tryserver. Let's see how it behaves.
(In reply to Iaroslav Sheptykin from comment #14)
> (In reply to Georg Fritzsche [:gfritzsche] [away Oct 6 - Oct 8] from comment
> #12)
> 
> Thanks for the review, Georg!
> 
> > Ok, i would have expected this to be based on my patch.
> 
> Sorry about that. I imported your patch and mixed it into my tree. I was not
> sure how reviewboard deals with such things and expected it to delete all
> earlier attachments to a bug just like it does when I update a review. So I
> smashed everything together with histedit and sent it to review.
> Just want you to know that there is no evil plan going on, rather a lack of
> experience. Next time I will try to do it properly.

No worries, i was just confused there for a moment - happens :)
Comment on attachment 8669564 [details]
MozReview Request: Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r=ted,gfritzsche

Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r=ted,gfritzsche
Attachment #8669564 - Attachment description: MozReview Request: Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r?gfritzsche → MozReview Request: Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r=ted,gfritzsche
Attachment #8669564 - Flags: review+ → review?(gfritzsche)
Some tests failed but they seem unrelated. There is a problem with healthreport on Windows, but it looks like this started before our patch.
Attachment #8669564 - Flags: review?(gfritzsche) → review+
Comment on attachment 8669564 [details]
MozReview Request: Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r=ted,gfritzsche

https://reviewboard.mozilla.org/r/21237/#review19521

Nothing really changed to the last version here?
ReviewBoard says there are some whitespace changes in `configure.in` in comparison to the previous revision.
Please double-check that you didn't unintentionally change some whitespacing in that file.
The failures look unrelated, i starred them as such.
Hi Georg! I didn't touch configure.in in the last changeset. I only updated the commit message with the reference to ted. 
As far as I remember I did not rebase my work onto the newest central so my changeset applies to an outdated version of configure.in. This could explain the confusion of the ReviewBoard. But I will double-check that.
Comment on attachment 8669564 [details]
MozReview Request: Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r=ted,gfritzsche

Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r=ted,gfritzsche
Attachment #8669564 - Flags: review+ → review?(gfritzsche)
Hi Georg!

It looks like we had a race with a bug 121452. I rebased our changeset onto the latest central which required a merge in moz.build. I checked the configure.in for empty lines introduced by our patch but I don"t see any. I believe that my histedits confuse reviewboard. Next time I will submit incremental changesets without editing published history. Sorry about that.
I am resending this to the tryserver.
Comment on attachment 8669564 [details]
MozReview Request: Bug 1209458: Replace HISTOGRAM_FILE_VERSION preprocessor usage with AppConstants.jsm definitions. r=ted,gfritzsche

https://reviewboard.mozilla.org/r/21237/#review19791

This looks good then, thanks for double-checking!
Attachment #8669564 - Flags: review?(gfritzsche) → review+
Hi Georg!

The tests seem allright to me:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=336fb73d4319

checkin-needed here?
Flags: needinfo?(gfritzsche)
Yes, those look fine, thanks!
Flags: needinfo?(gfritzsche)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34b1b7b2c871
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.