Allow overriding SOURCE_REPO, SOURCE_CHANGESET, SOURCE_STAMP

REOPENED
Unassigned

Status

Firefox Build System
General
REOPENED
2 years ago
2 months ago

People

(Reporter: Dexter, Unassigned)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45 affected, firefox46 affected, firefox47 affected)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Blocks: 1233687
Priority: -- → P2
Whiteboard: [measurement:client]
Assignee: nobody → gfritzsche
Points: --- → 1
Priority: P2 → P1
(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?
(Reporter)

Comment 2

2 years ago
(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?
(Reporter)

Comment 3

2 years ago
(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
Created attachment 8710432 [details] [diff] [review]
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 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?
Attachment #8710432 - Flags: review?(ted)
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 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.
Attachment #8710432 - Flags: review?(ted)
Points: 1 → 2
Created attachment 8711772 [details] [diff] [review]
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.
Attachment #8711772 - Flags: review?(ted)
Attachment #8710432 - Attachment is obsolete: true
Summary: Allow to define SOURCE_REV_URL when building outside from a mercurial repository → Allow overriding SOURCE_REV_URL, SOURCE_REPO, SOURCE_CHANGESET
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: :=
Attachment #8711772 - Flags: review?(ted) → review+
Created attachment 8712298 [details] [diff] [review]
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.
Attachment #8712298 - Flags: review?(ted)
Attachment #8711772 - Attachment is obsolete: true
Comment on attachment 8712298 [details] [diff] [review]
Allow overriding SOURCE_REV_URL, SOURCE_REPO, SOURCE_CHANGESET

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

Thanks!
Attachment #8712298 - Flags: review?(ted) → review+
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
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
Flags: needinfo?(jopsen)
Looks like i was pushing from a bad revision and it was unrelated to my changes.
Flags: needinfo?(jopsen)
Created attachment 8713714 [details] [diff] [review]
Allow overriding SOURCE_REV_URL with external repository information

This is a minimal version for uplift that just addresses what we need for Ubuntu.
Attachment #8712298 - Attachment is obsolete: true
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.
Attachment #8713714 - Flags: review+
Attachment #8713714 - Flags: approval-mozilla-beta?
Attachment #8713714 - Flags: approval-mozilla-aurora?
status-firefox44: --- → wontfix
status-firefox45: --- → affected
status-firefox47: --- → affected
Flags: qe-verify+
QA Contact: alexandra.lucinet

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8d8faa25955
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Comment 21

2 years ago
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla47 → ---
Depends on: 1244688
Taking the original task of overriding SOURCE_REV_URL to bug 1244688 so this can finally land and uplift.
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
Attachment #8713714 - Flags: approval-mozilla-beta?
Attachment #8713714 - Flags: approval-mozilla-beta-
Attachment #8713714 - Flags: approval-mozilla-aurora?
Attachment #8713714 - Flags: approval-mozilla-aurora-
Summary: Allow overriding SOURCE_REV_URL, SOURCE_REPO, SOURCE_CHANGESET → Allow overriding SOURCE_REPO, SOURCE_CHANGESET, SOURCE_STAMP
Priority: P1 → P2
No longer blocks: 1233687
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.
Assignee: gfritzsche → nobody
Priority: P2 → P3
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.
status-firefox47: fixed → affected
Component: Telemetry → Build Config
Priority: P3 → --
Product: Toolkit → Core
Whiteboard: [measurement:client]

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.