Simplify finding revision and repository information from source tarballs

RESOLVED FIXED

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [measurement:client:tracking])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
We distribute source code in tarballs from [1]. The tarballs are being used, among other things, to package Firefox builds inside Linux Distributions (e.g. Arch Linux).

To support some use cases (e.g. Telemetry sending revision info, about:buildconfig, etc.), we need them to package Firefox and add the revision information.

I thought that no revision information was being published in [1], but :rail pointed me to [2], which seems a bit hard to find without knowing it beforehand.

It would be great if we could publish revision information it in a better format under http://ftp.mozilla.org/pub/firefox/releases/47.0.1/

[1] - http://ftp.mozilla.org/pub/firefox/releases/47.0.1/source/
[2] - http://ftp.mozilla.org/pub/firefox/candidates/47.0.1-candidates/build1/linux-x86_64/en-US/firefox-47.0.1.txt
Assignee

Updated

3 years ago
Whiteboard: [measurement:client:tracking]
We can probably have something like http://ftp.mozilla.org/pub/firefox/candidates/47.0.1-candidates/build1/linux-x86_64/en-US/firefox-47.0.1.json, modulo architecture specific variables.
Assignee

Comment 2

3 years ago
(In reply to Rail Aliiev [:rail] from comment #1)
> We can probably have something like
> http://ftp.mozilla.org/pub/firefox/candidates/47.0.1-candidates/build1/linux-
> x86_64/en-US/firefox-47.0.1.json, modulo architecture specific variables.

That would be great. I actually just need this part:

  "moz_source_repo": "MOZ_SOURCE_REPO=https://hg.mozilla.org/releases/mozilla-release", 
  "moz_source_stamp": "7f5abf95991bda0bc2b8e0d774a8866b726b312b", 
  "moz_update_channel": "release",
Assignee

Updated

3 years ago
Blocks: 1285195
Assignee

Updated

3 years ago
Blocks: 1285201
Having a file inside the tarball with the information would be ideal. It would also allow the build system to read the info itself.
Put a file in the source tarball with the VCS info and we can update build/variables.py to read from it.

In other news, a build peer needs to spend an hour or two to consolidate all uses of MOZ_SOURCE_REPO and MOZ_SOURCE_CHANGESET in the tree. There is some duplication there...
bug 1244688 did some foundational work to make it possible to specify MOZ_SOURCE_{CHANGESET,REPO} when doing a build, so it should be straightforward to pull those from a known location.

There's almost certainly some things that could be cleaned up in the wake of that--bug 1247162 looks like it did a lot of good work to start that process.
Assignee

Comment 6

3 years ago
Is there any update on this?
Not really :/
Assignee

Comment 8

3 years ago
Doe(In reply to Rail Aliiev [:rail] from comment #7)
> Not really :/

Ouch :( Does anybody know where the script that generates the source tarball lives?
Not 100% sure here but the job that does that during the release process is [0]. Glancing at it, the script invoked looks to be [1] with these configs[2]. The fx_desktop_build script lies in-tree at [3] and the configs are at [4]. Looking at the configs I'd say "package-source" is the action that handles the actual source generation and can be found at [5] (FxDesktopBuild inherits it via BuildScript


[0]: https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/firefox/source.yml.tmpl#L10
[1]: https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/firefox/source.yml.tmpl#L48
[2]: https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/firefox/source.yml.tmpl#L49
[3]: http://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/scripts/fx_desktop_build.py
[4]: http://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/configs/builds/releng_sub_linux_configs/64_source.py
[5]: http://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/mozharness/mozilla/building/buildbase.py#l1771
In other words, we need to change "make source-package" to generate an extra file: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#222-227
We could reuse the existing code that generates the .txt and .json files that go next to the build, and just stick them in the source package:
https://dxr.mozilla.org/mozilla-central/rev/9baec74b3db1bf005c66ae2f50bafbdb02c3be38/toolkit/mozapps/installer/packager.mk#96

`make-sourcestamp-file` makes the .txt file and `make-buildinfo-file` makes the .json. They both seem to pull their info from source-repo.h now, which is fed by the `MOZ_SOURCE_{CHANGESET,REPO}` variables if present, so assuming the source packaging machinery has run configure that ought to work. We'd have to then change configure to look for whatever file we stick in the source package, and use that to feed the same code that currently reads the MOZ_SOURCE_* variables.

Updated

3 years ago
Duplicate of this bug: 1312408
Alessio, there are platform.ini and application.ini files in the root of the tarball. Don't those files contain the information that you need?
Flags: needinfo?(alessio.placitelli)
Assignee

Comment 14

3 years ago
(In reply to Jonathan Watt [:jwatt] from comment #13)
> Alessio, there are platform.ini and application.ini files in the root of the
> tarball. Don't those files contain the information that you need?

Hey Jonathan, thanks for following up on this and sorry for the delay. AFAIK these two files get populated by the build system and require either passing the revision information externally (see bug 1244688) or building from a directory with repository information.
Flags: needinfo?(alessio.placitelli) → needinfo?(jwatt)
Assignee

Comment 15

3 years ago
Posted patch bug1282782.patch (obsolete) — Splinter Review
Thanks Ted (and Rail), you were vital to get this done.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Flags: needinfo?(jwatt)
Attachment #8820827 - Flags: review?(ted)
Thank you for the patch!
Comment on attachment 8820827 [details] [diff] [review]
bug1282782.patch

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

A few nits, but it looks good!

::: build/variables.py
@@ +54,5 @@
> +
> +    # Load the content of the file.
> +    lines = None
> +    with open(sourcestamp_path) as f:
> +        lines = [l for l in f.read().splitlines()]

The list comprehension here is unnecessary, just `lines = f.read().splitlines()`.

@@ +78,5 @@
>      if not repo:
>          if os.path.exists(os.path.join(buildconfig.topsrcdir, '.hg')):
>              repo, changeset = get_hg_info(buildconfig.topsrcdir)
> +        elif os.path.exists(os.path.join(buildconfig.topsrcdir, SOURCESTAMP_FILENAME)):
> +            repo, changeset = get_info_from_sourcestamp(buildconfig.topsrcdir)

Might as well just put the full pathname from the `join` above in a variable and pass it to `get_info_from_sourcestamp`.

::: toolkit/mozapps/installer/packager.mk
@@ +226,5 @@
>  		$(CHECKSUM_FILES)
>  
>  # source-package creates a source tarball from the files in MOZ_PKG_SRCDIR,
>  # which is either set to a clean checkout or defaults to $topsrcdir
> +source-package: #make-sourcestamp-file

Remove the commented-out bit here. :)

@@ +236,2 @@
>  	@echo 'Packaging source tarball...'
> +	# We want to include the sourcestamp file in the source tarball, so copy it

An additional note as to *why* we want this would be good. "So we can submit telemetry from builds made from the source package with the correct revision information."
Attachment #8820827 - Flags: review?(ted) → review+
Assignee

Comment 18

3 years ago
Thanks Ted!
Attachment #8820827 - Attachment is obsolete: true
Attachment #8821210 - Flags: review+
Assignee

Comment 19

3 years ago
This was tested manually using the following command:

> ./mach build source-package

The "sourcestamp.txt" file was correctly added to the archive. I've then unpacked the generated archive and started a build from the extracted source. The revision information from the txt file were correctly picked up by the build system.

Marking this as checkin-needed, there's no need for a try-push as it wouldn't cover the changed functionalities.
Keywords: checkin-needed

Comment 20

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d511576146
Add sourcestamp file to the source package. r=ted
Keywords: checkin-needed

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7d511576146
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Assignee

Comment 22

3 years ago
:rail, do you have any idea of when this code will be used for the first time? Or, better, when are we supposed to generate the next source tarball? Uplift day?
Flags: needinfo?(rail)
It'll be used first in 53.0b1, which is according to https://wiki.mozilla.org/RapidRelease/Calendar in early March. The patch can be uplifted (requires approval‑mozilla‑aurora and approval‑mozilla‑beta from the Release Management team) if you want to see it earlier.
Flags: needinfo?(rail)
Assignee

Comment 24

3 years ago
(In reply to Rail Aliiev [:rail] ⌚️ET from comment #23)
> It'll be used first in 53.0b1, which is according to
> https://wiki.mozilla.org/RapidRelease/Calendar in early March. The patch can
> be uplifted (requires approval‑mozilla‑aurora and approval‑mozilla‑beta from
> the Release Management team) if you want to see it earlier.

Ah, thanks, I thought for some reason that the timings were different for this chunk of code :)
Assignee

Comment 25

3 years ago
Comment on attachment 8821210 [details] [diff] [review]
bug1282782.patch

Approval Request Comment
[Feature/Bug causing the regression]: This will allow third party packaged Firefox builds to send Telemetry: we will be able to add revision information to builds created off source tarballs. 
[User impact if declined]: None, this merely changes a portion of the build system.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: This was manually tested as reported in comment 19.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: This merely adds the sourcestamp.txt file to the source tarball packaged when releasing a new version of Firefox. It also changes the build system to read revision information from that file, if available, when building off a source tarball.
[Why is the change risky/not risky?]: This shouldn't be risky. Worst that can happen is that no sourcestamp.txt file gets generated and added to the source tarball archive. This would be the same state as before the patch landed, so no big deal.
[String changes made/needed]: None
Attachment #8821210 - Flags: approval-mozilla-beta?
Attachment #8821210 - Flags: approval-mozilla-aurora?
I'm wondering if this gives enough warning/testing time for the people using the 3rd party builds. Let's uplift to aurora now and I'll email or n-i to people from various distros. What do you think?
Comment on attachment 8821210 [details] [diff] [review]
bug1282782.patch

Let's uplift to aurora for now.
Attachment #8821210 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8821210 [details] [diff] [review]
bug1282782.patch

On 2nd thought let's put this in beta 11 and see if we get feedback.
Attachment #8821210 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Comment 29

3 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> I'm wondering if this gives enough warning/testing time for the people using
> the 3rd party builds. 

Thanks Liz! Anyway, I don't think this should have an impact on 3rd party builds, as they'd need to enable other stuff as well in order to enable Telemetry (it's more of a requirement to enable Telemetry).

> Let's uplift to aurora now and I'll email or n-i to people from various distros. What do you think?

That's interesting! We should definitely sync up/get in touch next year so that we can build a list of 3rd party distributors to contact to enable Telemetry!
Firefox 51 tarballs now have sourcestamp.txt files, but they only contain a linebreak.
Assignee

Comment 33

2 years ago
(In reply to Jan Steffens from comment #32)
> Firefox 51 tarballs now have sourcestamp.txt files, but they only contain a
> linebreak.

Thanks Jan, we're investigating that.
Assignee

Updated

2 years ago
Depends on: 1338099
You need to log in before you can comment on or make changes to this bug.