Closed Bug 549958 Opened 14 years ago Closed 13 years ago

Hourly/Nightly builds should have some way to see which {comm-central|mobile-browser|camino} changeset was used, too

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(status1.9.2 .17-fixed, status1.9.1 .19-fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b12
Tracking Flags:
Tracking Status
status1.9.2 --- .17-fixed
status1.9.1 --- .19-fixed

People

(Reporter: sgautherie, Assigned: alqahira)

References

Details

Attachments

(2 files, 1 obsolete file)

Implements comment 6 + comment 8
3.44 KB, patch
ted
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
Roll-up patch for branches
3.43 KB, patch
dveditz
: approval1.9.2.17+
dveditz
: approval1.9.1.19+
Details | Diff | Splinter Review 
Bug 474610 comment 17:
{
Justin Wood (irc: Callek)      2010-02-27 21:30:11 PST

I suspect we may want a slightly different solution for c-c here (package a
"list" of changesets perhaps)
}

***

Current code is at:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/installer/packager.mk#440
{
436 make-package: stage-package $(PACKAGE_XULRUNNER)
440         @echo "$(BUILDID) $(MOZ_SOURCE_STAMP)" > $(DIST)/$(PKG_PATH)/$(PKG_BASENAME).txt
}
Flags: in-testsuite-
Attached patch Thought, very rough poc hack (obsolete) — — Splinter Review 
Here's an idea that I had for how to attack this (it appears to work, but I haven't checked all my Ps and Qs to make sure it will always do what I think it does):

1) In packager.mk, change the make-package target to depend on a new target, make-sourcestamp-txt (with a better name, of course).

2) make-sourcestamp-txt now calls the rule Ted added in bug 474610.  make-sourcestamp-txt is also a :: target.  Assuming all my Ps and Qs are in order, this is essentially a no-op for single-repo products (Firefox, XR).

Then, every product that has its own repo can add a make-sourcestamp-txt:: rule to its installer/Makefile.in (or equivalent), which gets called along with the rule in packager.mk.

So, for instance, Camino (camino/installer/Makefile.in) could have a target/rule that looks like this:

make-sourcestamp-txt::
 	@echo "$(CM_SOURCE_STAMP)" >> $(DIST)/$(PKG_PATH)/$(PKG_BASENAME).txt

Or, if we wanted, we could modify (override) the output to echo the repo name

make-sourcestamp-txt::
 	@echo "$(BUILDID) $(MOZ_SOURCE_STAMP) $(MOZ_SOURCE_REPO)" > $(DIST)/$(PKG_PATH)/$(PKG_BASENAME).txt
 	@echo "$(CM_SOURCE_STAMP) $(CM_SOURCE_REPO" >> $(DIST)/$(PKG_PATH)/$(PKG_BASENAME).txt

This setup (the pair of make-sourcestamp-txt:: targets, in Core and in the app) is both flexible so that apps can do what they need to make their sourcestamp file useful and understandable, but also at the same time preserves the benefit of the shared packager.mk rule for single-repo apps or multi-repo apps that might not want to mess with changing this now.
I know you filed this specifically about c-c apps, but since this bug lives in Core:Build Config and not Mailnews Core:Build Config and the problem applies to everyone with app-specific repos, let's broaden the summary to include other relevant repos.  (Not sure who to CC here from fennec, though.)
Summary: Hourly/Nightly builds should have some way to see which comm-central changeset was used too → Hourly/Nightly builds should have some way to see which {comm-central|mobile-browser|camino} changeset was used, too
(In reply to comment #1)
> 2) make-sourcestamp-txt now calls the rule Ted added in bug 474610. 
> make-sourcestamp-txt is also a :: target.  Assuming all my Ps and Qs are in
> order, this is essentially a no-op for single-repo products (Firefox, XR).

"this" = "splitting the @echo rule into its own target and having make-package depend on the new target"
I like the general idea of this. What I'd really like to see us be able to do is to produce a file like:

mozilla-central 1234567890
comm-central 1357924680
(and maybe other revisions as appropriate).

Hence we'd actually be able to get exactly which revisions were used (frequently, having the m-c revision as well as the c-c helps us a lot).

I've not looked at where this is used to know what a format change like this might break.
(In reply to comment #4)
> I've not looked at where this is used to know what a format change like this
> might break.

I don't think it's being used elsewhere yet, though I haven't attempted to parse the automation code to verify that.  Bug 548549 does want to do something with it, though.

Maybe we should try to standardize on a format like:

$BuildID
$PlatformRepoName $PlatformChangest
$AppRepoName1     $AppChangeset1
$AppRepoName2     $AppChangeset2
$AppRepoName3     $AppChangeset3
…

where the first two lines are required (and produced by packager.mk) and any additional lines are optional (and are produced by the target's pair in the app's installer Makefile)?
I kind of like the simplicity of the current Firefox .txt files, but I'd settle for:
$BUILDID
$REPO/rev/$CHANGESET
...

so like:
20100426010203
http://hg.mozilla.org/mozilla-central/rev/abcd1234
http://hg.mozilla.org/comm-central/rev/fedc4321
Also your implementation suggestion sounds sane.
Ted, thanks for the feedback :)  I had also started thinking that maybe the rev-URL would be slightly more useful; it's certainly easier to generate.

I've already renamed "make-sourcestamp-txt" to "make-sourcestamp-file" locally; maybe it should even be "make-sourcestampfile" in keeping with the other make-foo targets (though that gets to be a bit long).

The other thing I think we should do is make "$(DIST)/$(PKG_PATH)/$(PKG_BASENAME).txt" its own variable, so that the half-dozen-or-so places across the trees that are going to be dealing with this file won't have to all be modified if anyone ever change the file's filename (there was some talk of that in another bug at one point).

I'll see about getting a working patch up this week.
Assignee: nobody → alqahira
Status: NEW → ASSIGNED
Apparently Socorro is now scraping this data:
http://groups.google.com/group/mozilla.dev.planning/msg/ddf8c7234a4b74a7

So we'll have to be careful about changing the format of the file that Firefox uploads. The best plan would probably be to file a Socorro bug to change the scraping script it's using to handle both the old format and the new format, and get that landed and in production before landing any changes here.
Filed bug 562114 on comment 9; thanks for the heads-up on that, too!
Here's a patch that implements what we've discussed.  

The only really new change here is fixing the MOZ_SOURCE_STAMP variable definition to use $(MOZILLA_DIR) instead of $(topsrcdir).  This is done to make sure that packager.mk's make-sourcetamp-file rule always generates the platform changeset, so that the file format will remain consistent about what's on the first two lines (build id, platform changeset) no matter what the repo layout is.  With the current code in the tree, in comm-*, packager.mk's rule is actually generating "BuildID comm-*-changeset" for the file's content (according to SeaMonkey; Tb doesn't seem to be uploading them, for whatever reason), so switching to $(MOZILLA_DIR) should fix that.

I don't understand the .ini files (or maybe how $(topsrcdir) changes meaning in a comm-* build), but somehow the same sourcestamp rule in toolkit/xre/Makefile.in appears to be generating the right sourcestamp for platform.ini using $(topsrcdir), even though it seems like it should be returning the wrong thing there, too.
Attachment #438923 - Attachment is obsolete: true
Attachment #442577 - Flags: review?(ted.mielczarek)
(In reply to comment #11)

> The only really new change here is fixing the MOZ_SOURCE_STAMP variable
> definition to use $(MOZILLA_DIR) instead of $(topsrcdir).

You may want to add a comment about that in the file.

> With the current code in the tree, in comm-*, packager.mk's rule is
> actually generating "BuildID comm-*-changeset" for the file's content
> (according to SeaMonkey

(Indeed ;-|)

> I don't understand the .ini files (or maybe how $(topsrcdir) changes meaning in
> a comm-* build), but somehow the same sourcestamp rule in
> toolkit/xre/Makefile.in appears to be generating the right sourcestamp for
> platform.ini using $(topsrcdir), even though it seems like it should be
> returning the wrong thing there, too.

It does because packager.mk is included into c-c makefiles,
whereas toolkit/xre/Makefile.in is run from within m-c.
Comment on attachment 442577 [details] [diff] [review]
Implements comment 6 + comment 8

>diff --git a/toolkit/mozapps/installer/package-name.mk b/toolkit/mozapps/installer/package-name.mk
>--- a/toolkit/mozapps/installer/package-name.mk
>+++ b/toolkit/mozapps/installer/package-name.mk
>+# strip a trailing slash from the repo URL because it's not always present,
>+# and we want to construct a working URL in the sourcestamp file.
>+# make+shell+sed = awful
>+_dollar=$$
>+MOZ_SOURCE_REPO := $(shell cd $(MOZILLA_DIR) && hg showconfig paths.default 2>/dev/null | head -n1 | sed -e "s/^ssh:/http:/" -e "s/\/$(_dollar)//" )

Use =, not :=, otherwise this will get evaluated in every makefile that includes this file, instead of just the one place where it's used.

r=me with that fix, although obviously we shouldn't land this until the blocking bug is fixed, so as to not break Socorro.
Attachment #442577 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #13)
> Use =, not :=, otherwise this will get evaluated in every makefile that
> includes this file, instead of just the one place where it's used.
> 
> r=me with that fix, although obviously we shouldn't land this until the
> blocking bug is fixed, so as to not break Socorro.

Thanks, Ted; fixed locally in anticipation of that day coming at some point :)
Comment on attachment 442577 [details] [diff] [review]
Implements comment 6 + comment 8

Now that Socorro 1.8 has finally deployed, requesting approval2.0 on this build-config-only change that allows the sourcestamp info file produced by the build system to be useful for other-than-Firefox apps.  This file does not ship with the application and has no effect on application code.

(We'll want this on the branches, too, but branch-drivers will still probably be happier if it's landed on trunk first.)
Attachment #442577 - Flags: approval2.0?
Comment on attachment 442577 [details] [diff] [review]
Implements comment 6 + comment 8

Socorro's being rolled back to 1.7 :(  They hope to go back to 1.8 in a week or two, so I'm not going to cancel the approval request (it may take that long for someone to get to it); I just won't land until Socorro's back to 1.8.  Since this bug isn't a blocker or anything, I hope doing that will not screw up queries; if it does, I guess I'll just forego approval until Socorro's back to 1.8 :(
Attachment #442577 - Flags: approval2.0? → approval2.0+
And Socorro finally went forward to 1.7.whatever-had-bug-562114-in-it over the weekend, with no signs of being rolled back :)

http://hg.mozilla.org/mozilla-central/rev/0d93633ce739

I'd still like to get this on the branches (like the sourcestamp file introduction was), to help with branch regressions and whatnot for apps there; I'll wait a few days to make sure there are no unexpected consequences on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/comm-central/rev/ce2bbf8d11fd
Use a locally generated version of source stamp for now, not the exported one.
I'm confused about that bustage; is it because comm-central was reusing the same MOZ_SOURCE_STAMP variable to mean the comm-central revision rather than the platform one?  (Except for Calendar, which was using MOZILLA_SRCDIR rather than topsrcdir in its rule, so it apparently wanted the platform changeset.)

Sorry about that; I'll be sure to give advanced notice of any checkins on the branch(es) to avoid this again :(
(In reply to comment #19)
> I'm confused about that bustage; is it because comm-central was reusing the
> same MOZ_SOURCE_STAMP variable to mean the comm-central revision rather than
> the platform one?  (Except for Calendar, which was using MOZILLA_SRCDIR rather
> than topsrcdir in its rule, so it apparently wanted the platform changeset.)

So the MOZ_SOURCE_STAMP is exported via the main Makefile. i.e. comm-central/Makefile.in or mozilla/Makefile.in

At that time, the MOZ_SOURCE_STAMP variable was calculated via package-name.mk by the build system that was built.

So for comm-central/Makefile.in the $(topsrcdir) variable would equate to '.', when building the mozilla-central part, mozilla/Makefile.in would equate to mozilla/ in a comm-central set-up.

The patch changed it to $(MOZILLA_SRCDIR) which is always mozilla/ in the comm-central set-up whether or not you're in the mozilla directory.

The bustage fix changes the way we rely on the MOZ_SOURCE_STAMP variable so that we'll always re-generate it and not rely on the exported variable.

> Sorry about that; I'll be sure to give advanced notice of any checkins on the
> branch(es) to avoid this again :(

No problem.
I posted http://www.ardisson.org/afkar/2011/01/28/sourcestamp-file-format-changed-on-mozilla-central/ to hopefully head off any other lurking issues with tools or code depending on the format of the sourcestamp file.
Blocks: 630176
Blocks: 630487
This caused build failure in Firefox4.0beta11 build#1. This change now backed out of relbranch, and we're now starting Firefox4.0beta11 build#2.

bug#631006 filed to track fixing bustage for next time.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #22)
> This caused build failure in Firefox4.0beta11 build#1. This change now backed
> out of relbranch, and we're now starting Firefox4.0beta11 build#2.
> 
> bug#631006 filed to track fixing bustage for next time.

Duh. Meant to leave this bug#549958 closed, because new bug#631006 already filed to track fixup work.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
This is a roll-up patch for the branches, which is attachment 442577 [details] [diff] [review] plus Gavin's PRETTYNAMES fix in bug 631006.  Fx 4.0 Beta 12 included both and had no sourcestamp file-related build issues, so the combination of the two fixes now has been tested in release situations and should be safe for branches.

Landing this on the branches will allow tools depending on this file (e.g. Socorro) to no longer have to support both old and new sourcestamp file formats and will allow non-Firefox products on the branch to include their repository and changeset info in the sourcestamp file to aid in build identification and regression hunting.
Attachment #514861 - Flags: approval1.9.2.15?
Attachment #514861 - Flags: approval1.9.1.18?
Comment on attachment 514861 [details] [diff] [review]
Roll-up patch for branches

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz
Attachment #514861 - Flags: approval1.9.2.15?
Attachment #514861 - Flags: approval1.9.2.15+
Attachment #514861 - Flags: approval1.9.1.18?
Attachment #514861 - Flags: approval1.9.1.18+
standard8 (and anyone else watching this bug for notice of the branch landings):

I'd like to land this on the branches some late afternoon or evening (GMT-5) this week (but not this afternoon/evening, obviously); can we coordinate so that the comm-* changes are ready for someone to push (or they can even go in beforehand, right, and then I could land once I've heard they're in)?
(In reply to comment #26)
> I'd like to land this on the branches some late afternoon or evening (GMT-5)
> this week (but not this afternoon/evening, obviously); can we coordinate so
> that the comm-* changes are ready for someone to push (or they can even go in
> beforehand, right, and then I could land once I've heard they're in)?

I've just done this on comm-1.9.2:

http://hg.mozilla.org/releases/comm-1.9.2/rev/11a5d8031d8c

with a=me.

(just a straight landing of http://hg.mozilla.org/comm-central/rev/ce2bbf8d11fd).

You'll need one of the SeaMonkey team to do comm-1.9.1 as Thunderbird is no longer active there (EOLed).
(In reply to comment #27)
> You'll need one of the SeaMonkey team to do comm-1.9.1 as Thunderbird is no
> longer active there (EOLed).

From a skim I don't see anything necessary for SeaMonkey to cope with this on the 1.9.1 branch, it looks like we used a different method there that is not conflicting.

If anything breaks for us there after this lands on branch, I feel safe to deal with it in a new bug.
Thanks, gentlemen.  I'll aim for this evening.
The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: