1241111 - Allow overriding SOURCE_REPO, SOURCE_CHANGESET, SOURCE_STAMP

Status: REOPENED

(Firefox Build System :: General, defect)

Description

[Alessio Placitelli [:Dexter]]

When building Firefox outside of a mercurial repository (e.g. from source trees cloned off the official hg repo), we loose the SOURCE_REV_URL [0] definition.

This will prevent us from sending the correct Histogram.json revision [1] along with the main ping [2].

We should change configure.in [0] and check for non empty SOURCE_REV_URL, eventually defining it if not available.

[0] - https://dxr.mozilla.org/mozilla-central/rev/a77b73c7723e1060993045fb31eb2f0a30473486/configure.in#8742
[1] - https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/toolkit/components/telemetry/TelemetrySession.jsm#954
[2] - https://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/main-ping.html

Comment 1

[Mike Hommey [:glandium]]

(In reply to Alessio Placitelli [:Dexter] from comment #0)
> 
> We should change configure.in [0] and check for non empty SOURCE_REV_URL,
> eventually defining it if not available.

defining it to what?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Mike Hommey [:glandium] from comment #1)
> > We should change configure.in [0] and check for non empty SOURCE_REV_URL,
> > eventually defining it if not available.
> 
> defining it to what?

AFAICT the SOURCE_REV_URL is defined/set to "empty" in the else branch?

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #2)
> (In reply to Mike Hommey [:glandium] from comment #1)
> > > We should change configure.in [0] and check for non empty SOURCE_REV_URL,
> > > eventually defining it if not available.
> > 
> > defining it to what?
> 
> AFAICT the SOURCE_REV_URL is defined/set to "empty" in the else branch?

https://dxr.mozilla.org/mozilla-central/rev/a77b73c7723e1060993045fb31eb2f0a30473486/configure.in#8747

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Allow overriding SOURCE_REV_URL

Holding review until it is clear how exactly we proceed with Ubuntu Telemetry. Wondering if we have a standard for defines used from mozconfigs et al, like prefixing with MOZ_?

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8710432 [details] [diff] [review]
Allow overriding SOURCE_REV_URL

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

We are fixing Telemetry being sent from Ubuntu & found that Ubuntu packaging drops the HG source information.
However, we still need to have SOURCE_REV_URL set so we can identify the correct Histograms.json revision for Telemetry submissions.
So, we allow to define SOURCE_REV_URL with an external value and they can set that explicitly from their build.

Does that sound ok?
Should we name the override differently, say MOZ_SOURCE_REV_URL?

Comment 6

[(not currently active) Ted Mielczarek]

We actually hit a similar issue in bug 1231618 with Taskcluster builds. They use `tc-vcs` to clone, but that tool will clone a base repository (usually mozilla-central) and then pull whatever branch is being built into the repo, so `hg showconfig paths.default` gives the wrong repo.

bug 1231618 comment 1 lists a few other places where we're currently gathering repo/rev info from hg directly. Do you think you could fix those all while you're here? Using your patch as a base to fix configure and then just changing all the other places to use the info gathered from configure would be fine. Right now none of this stuff works properly in a non-clobber build, but since we don't ship those to users I'm not particularly worried about it.

If we're going to have people pass this in from outside the build then prefixing it with MOZ_ might make sense.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8710432 [details] [diff] [review]
Allow overriding SOURCE_REV_URL

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

Just clearing this per my previous comment, but in theory this is fine.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Allow overriding SOURCE_REV_URL

How is this? I checked AppConstants.jsm, application.ini and buildconfig.html to behave properly over unofficial builds and official with and without overriding REPO and CHANGESET.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8711772 [details] [diff] [review]
Allow overriding SOURCE_REV_URL

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

Looks good, thanks! There's one other place we use this info, in the Breakpad symbols. That gets set here:
https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/toolkit/crashreporter/tools/symbolstore.py#139

We've already got an environment variable override for the repo URL, you could easily fix that to honor the substs for both, like:
```
if 'MOZ_SOURCE_REPO' in buildconfig.substs:
 ...
```

::: build/Makefile.in
@@ +30,3 @@
>  ifdef MOZ_INCLUDE_SOURCE_INFO
> +ifdef MOZ_SOURCE_REPO
> +source_repo = $(MOZ_SOURCE_REPO)

nit: :=

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Allow overriding SOURCE_REV_URL, SOURCE_REPO, SOURCE_CHANGESET

I am not too familiar with our Python build script style, i would prefer you having another quick look.

Comment 11

[Georg Fritzsche [:gfritzsche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db3740d1272f

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 8712298 [details] [diff] [review]
Allow overriding SOURCE_REV_URL, SOURCE_REPO, SOURCE_CHANGESET

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

Thanks!

Comment 13

[Georg Fritzsche [:gfritzsche]]

Due to complications with symbolstore.py i broke that out into bug 1243747 so the Ubuntu part is unblocked.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75c7d45a1e8c

Comment 14

[Georg Fritzsche [:gfritzsche]]

Jonas, do you know whats going wrong here with the tc tier 2 builds?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75c7d45a1e8c&selectedJob=16048180
https://treeherder.mozilla.org/logviewer.html#?job_id=16048180&repo=try#L81

Presumably this the failure:
> + for cache in /home/worker/.tc-vcs /home/worker/workspace /home/worker/tooltool-cache
> + chown -R worker:worker /home/worker/.tc-vcs
> chown: cannot access `/home/worker/.tc-vcs': No such file or directory

Comment 15

[Georg Fritzsche [:gfritzsche]]

Looks like i was pushing from a bad revision and it was unrelated to my changes.

Comment 16

[Georg Fritzsche [:gfritzsche]]

This looks fine now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f6ccdf5a793&selectedJob=16093840

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e8d8faa25955

Comment 18

[Georg Fritzsche [:gfritzsche]]

Attachment: Allow overriding SOURCE_REV_URL with external repository information

This is a minimal version for uplift that just addresses what we need for Ubuntu.

Comment 19

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8713714 [details] [diff] [review]
Allow overriding SOURCE_REV_URL with external repository information

Approval Request Comment
[Feature/regressing bug #]: Data reporting / Telemetry
[User impact if declined]: This is required for correct Telemetry data from Ubuntu.
[Describe test coverage new/current, TreeHerder]: fine on try, took this through manual testing scenarios
[Risks and why]: Low-risk, this version is very contained to just changing one define that is only used for Telemetry.
[String/UUID change made/needed]: None.

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e8d8faa25955

Comment 21

[Axel Hecht [:Pike]]

Seems that this broke single-locale android repacks:

05:29:55     INFO - Reading from file tmpfile_stdout
05:29:55     INFO -  gecko_revision OZ_SOURCE_CHANGESET
05:29:55     INFO - fennec_revision b2a3dc4b161fdfac6db78cc5bbfb1f53a383b65a
05:29:55     INFO - buildid 20160131030347

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-job_group_symbol=L10n&filter-job_group_symbol=Update-3&filter-job_group_symbol=Update-1&filter-job_group_symbol=Update-2&exclusion_profile=false is my treeherder view to actually show those.

Comment 22

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/0512df61134e for the rather more visible effect of completely breaking funsize, which I think is the cutely-named process that generates partial updates.

Apparently one of the things this needs in order to reland is the first-ever test that checks whether platform.ini actually contains a SourceStamp which looks like a cset rather than "OZ_SOURCE_CHANGESET", and a SourceRepository.

Comment 23

[Georg Fritzsche [:gfritzsche]]

Taking the original task of overriding SOURCE_REV_URL to bug 1244688 so this can finally land and uplift.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8713714 [details] [diff] [review]
Allow overriding SOURCE_REV_URL with external repository information

Original patch backout, please resubmit an uplift request when it landed in m-c

Comment 25

[Georg Fritzsche [:gfritzsche]]

I don't have time right now to investigate comment 21 and comment 22.
I can probably pick this up again later, but would be happy to have someone else pick it up.

Comment 26

[Georg Fritzsche [:gfritzsche]]

We solved the problem we have for Telemetry in bug 1244688.
I am not sure if we still need this one, moving to build config for triaging.