(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`.

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

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.

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?

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.)

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.

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

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.

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

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

Some tests failed but they seem unrelated. There is a problem with healthreport on Windows, but it looks like this started before our patch.

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

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!

Hi Georg! The tests seem allright to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=336fb73d4319 checkin-needed here?

Yes, those look fine, thanks!