Closed
Bug 1282782
Opened 8 years ago
Closed 8 years ago
Simplify finding revision and repository information from source tarballs
Categories
(Release Engineering :: Release Automation: Other, defect)
Release Engineering
Release Automation: Other
Tracking
(firefox51 fixed, firefox52 fixed, firefox53 fixed)
RESOLVED
FIXED
People
(Reporter: Dexter, Assigned: Dexter)
References
Details
(Whiteboard: [measurement:client:tracking])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Dexter
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Whiteboard: [measurement:client:tracking]
Updated•8 years ago
|
Blocks: release-promotion
Comment 1•8 years ago
|
||
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•8 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",
Comment 3•8 years ago
|
||
Having a file inside the tarball with the information would be ideal. It would also allow the build system to read the info itself.
Comment 4•8 years ago
|
||
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...
Comment 5•8 years ago
|
||
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•8 years ago
|
||
Is there any update on this?
Comment 7•8 years ago
|
||
Not really :/
Assignee | ||
Comment 8•8 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?
Comment 9•8 years ago
|
||
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
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
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•8 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•8 years ago
|
||
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)
Comment 16•8 years ago
|
||
Thank you for the patch!
Comment 17•8 years ago
|
||
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•8 years ago
|
||
Thanks Ted!
Attachment #8820827 -
Attachment is obsolete: true
Attachment #8821210 -
Flags: review+
Assignee | ||
Comment 19•8 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•8 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•8 years ago
|
||
bugherder |
Assignee | ||
Comment 22•8 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)
Comment 23•8 years ago
|
||
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•8 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•8 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•8 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!
Comment 30•8 years ago
|
||
bugherder uplift |
status-firefox52:
--- → fixed
Comment 31•8 years ago
|
||
bugherder uplift |
status-firefox51:
--- → fixed
Comment 32•8 years ago
|
||
Firefox 51 tarballs now have sourcestamp.txt files, but they only contain a linebreak.
Assignee | ||
Comment 33•8 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•