There should be enough information inside each build to determine what version of the source code was used for that build. I'm going to reference Firefox in this comment, but other projects may want this as well. I'm thinking that application.ini would be an appropriate place for this, but if there's somewhere more appropriate that's totally fine with me. I'm specifically thinking of things like performance testing where it'd be very useful to be able to pinpoint the checkins that went into a build without having to look elsewhere. The application.ini for Firefox currently specifies a "BuildID" which (for nightlies) is derived from the timestamp used for the build (but trimmed to the nearest hour), while for releases represents the time the build was started. For CVS-based builds, it would probably make the most sense to track the following information: Branch=MOZILLA_1_9_BRANCH Timestamp=1197019556 For Mercurial we'd probably need repo and revision.

This is my first patch but I know that it does not work yet, since I have problems with this line: |MOZ_SOURCE_STAMP = $(shell echo $MOZ_CO_DATE) |DEFINES += -DMOZ_SOURCE_STAMP=$(MOZ_SOURCE_STAMP) which generates this in the application.ini | SourceStamp=OZ_CO_DATE

I decided to call it SourceStamp since it seems to me that it conveys that idea between cvs and hg. I do not mention time because that would exclude the concept that hg does not use date/time stamp but a change set Another naming could SourceRevision instead

It WFM, I have to see what is the result if there is no MOZ_CO_DATE before running it Ted, to make this patch to go to the code, would you require it a patch that works for hg as well or could it go in before that? BTW, how do I ask if an environment variable is defined inside of a Makefile? any special syntax?

I'm not sure how the Mercurial vs. CVS thing is going to work. Patches landing in CVS now need special approval, and one of the conditions is generally that you've landed it in mercurial first. However, since your code is going to be different for CVS vs. Mercurial... I don't know what the policy is. Environment variables are just treated like any other variable in GNU Make. You can test if something is defined by using ifdef, like: ifdef MOZ_CO_DATE # do something endif

Comment on attachment 326816 [details] [diff] [review] Makefile.in - it really adds the source stamp to application.ini I have canceled since it does set to empty string if MOZ_CO_DATE has been set. I will work on it

Comment on attachment 326816 [details] [diff] [review] Makefile.in - it really adds the source stamp to application.ini I think that ifdef MOZ_CO_DATE DEFINES += -DMOZ_SOURCE_STAMP=$(MOZ_CO_DATE) endif should be good enough. See http://www.gnu.org/software/automake/manual/make/Environment.html#Environment and friends, too.

(In reply to comment #6) > (From update of attachment 326816 [details] [diff] [review]) > I think that > > ifdef MOZ_CO_DATE > DEFINES += -DMOZ_SOURCE_STAMP=$(MOZ_CO_DATE) > endif > We would be generating some builds with an empty string in the application.ini I don't think that would be good I have to find what is the timestamp that cvs checks out when I do not specify a MOZ_CO_DATE to be able to have a value I have tried this: |ifdef $(MOZ_CO_DATE) | DEFINES += -DMOZ_SOURCE_STAMP="$(shell echo $(MOZ_CO_DATE))" |else | DEFINES += -DMOZ_SOURCE_STAMP="$(shell date +"%m/%d/%Y %H:%M +0000")" |endif | but this would generate the source stamp according to the moment that the make -f client.mk build reaches that point and not when the source was checked out Aside of that, what would be the value to put in there if the build is generated from a release instead of a nightly?

This is my final patch, it meets the needs to know the SourceStamp in *CVS* which can be used by L10n to do repackages. At least this is what I need for Nighlies. I do not know if I need a 1) TAG and/or 2) BRANCH instead of a SourceStamp when we do a RELEASE, but will know when I reach there This patches do this: 1) If MOZ_CO_DATE is defined, which is used when running make -f client.mk checkout to checkout the specified date and late when we reach make -f client.mk build, we will have a line SourceStamp with the checkout date 2) If MOZ_CO_DATE is not defined, it will be as if have never had this patch, no extra line in application.ini, no changes, no other effects If we wanted to have *always* the line SourceStamp in application.ini, we would have to modify somewhere around http://mxr.mozilla.org/mozilla/source/client.mk#497 On HG, if checked out latest, we can still ask hg which changeset it did checkout. Therefore, the scenario to bring this extra line in mozilla-central would be even easier

Comment on attachment 326955 [details] [diff] [review] application.ini - If MOZ_CO_DATE set then add the line for the SourceStamp This seems reasonable, but I wonder if it's enough information? It doesn't capture branch information or anything like that. For Mercurial, I expect we'll have the same problem, wanting to know what repository it came off of. (mozilla-central, actionmonkey-central, etc).

Comment on attachment 326956 [details] [diff] [review] Makefile.in - It defines MOZ_SOURCE_STAMP if MOZ_CO_DATE is defined You don't need the $(shell echo ) bits in there, just DEFINES += -DMOZ_SOURCE_STAMP="$(MOZ_CO_DATE)" It's a make variable, so you can use it directly. (It just happens that make variables also wind up as environment variables.) Also, when you update this patch, can you combine these two patches into one updated patch? That makes it easier to review and easier for someone to checkin for you.

(In reply to comment #11) > (From update of attachment 326955 [details] [diff] [review]) > This seems reasonable, but I wonder if it's enough information? It doesn't > capture branch information or anything like that. > The title of the bug mentions more than I need, I just need the timestamp For now, the Source Stamp is good enough for what we need it for, if at a later point we have good reasons to have more info I know now how to do it. > For Mercurial, I expect we'll > have the same problem, wanting to know what repository it came off of. > (mozilla-central, actionmonkey-central, etc). > I could work on it later on when I start working on hg/l10n/build(In reply to comment #12) > (From update of attachment 326956 [details] [diff] [review]) > You don't need the $(shell echo ) bits in there, just > DEFINES += -DMOZ_SOURCE_STAMP="$(MOZ_CO_DATE)" > > It's a make variable, so you can use it directly. (It just happens that make > variables also wind up as environment variables.) > > Also, when you update this patch, can you combine these two patches into one > updated patch? That makes it easier to review and easier for someone to checkin > for you. > Yeah, I have realized in the last days that if 2 patches have high cohesion should go together, even they are in 2 different files

Comment on attachment 327877 [details] [diff] [review] application.ini + Makefile.in - Add source stamp to application.ini Looks good.

Comment on attachment 327877 [details] [diff] [review] application.ini + Makefile.in - Add source stamp to application.ini This patch allows to have the SourceStamp in application.ini in a build (if MOZ_CO_DATE has been set), this will help to not only identify a build by when it was created but to allow to know which source code was used. This can be reused with L10n repackages to know which en-US source code to check out and it will help out to know which source code was used in a build without having to go to the build's log. This should not be have any further implications.

(In reply to comment #4) > I'm not sure how the Mercurial vs. CVS thing is going to work. Patches landing > in CVS now need special approval, and one of the conditions is generally that > you've landed it in mercurial first. However, since your code is going to be > different for CVS vs. Mercurial... I don't know what the policy is. Well, how different is the code? If it's just a few arguments to differentiate between CVS and hg, the code should be mostly similar. Has a similar patch landed in hg? Which one and where?

(In reply to comment #17) > (In reply to comment #4) > > I'm not sure how the Mercurial vs. CVS thing is going to work. Patches landing > > in CVS now need special approval, and one of the conditions is generally that > > you've landed it in mercurial first. However, since your code is going to be > > different for CVS vs. Mercurial... I don't know what the policy is. > > Well, how different is the code? In mercurial the state of the repository is a changeset number rather than a date. Storing the date in application.ini for mercurial would have less meaning than in CVS - unless I ask around and find how out of a source stamp I could find out which changeset was the one used for that timestamp > Has a similar patch landed in hg? Which one and where? No, there is nothing working on hg > If it's just a few arguments to differentiate > between CVS and hg, the code should be mostly similar. As you say, it should be more or less similar

Comment on attachment 327877 [details] [diff] [review] application.ini + Makefile.in - Add source stamp to application.ini Talked this over with Armen... Approved for 1.9.0.2. Please land in CVS. a=ss

This needs to land today or it won't make it at all.

It appears that this patch is meant for branch? Please note that when marking checkin-needed, had I not looked closer I might have landed on trunk. It also looks like this is past the freeze date so this cannot land as it stands I think.

(In reply to comment #21) > It appears that this patch is meant for branch? Please note that when marking > checkin-needed, had I not looked closer I might have landed on trunk. It also > looks like this is past the freeze date so this cannot land as it stands I > think. > No, this patch is for trunk in CVS and only for CVS Firefox 3.0.2 has been postponed if I am not mistaken since we are doing a major update from ff2 to ff3.0.1 so it is fine to land to CVS Thanks

"trunk in CVS" == 1.9 *branch*. mozilla-central is known as trunk in development talk. Calling CVS HEAD the "trunk" is far too confusing.

Landed on branch Checking in browser/app/Makefile.in; /cvsroot/mozilla/browser/app/Makefile.in,v <-- Makefile.in new revision: 1.157; previous revision: 1.156 done Checking in browser/app/application.ini; /cvsroot/mozilla/browser/app/application.ini,v <-- application.ini new revision: 1.12; previous revision: 1.11 done

I've backed this out since it made linux and windows build machines fail.

gmake[5]: Entering directory `/builds/tinderbox/Fx-Trunk/Linux_2.6.18-53.1.13.el5_Depend/mozilla/obj-fx-trunk/browser/app' Makefile:75: *** invalid syntax in conditional. Stop. gmake[5]: Leaving directory `/builds/tinderbox/Fx-Trunk/Linux_2.6.18-53.1.13.el5_Depend/mozilla/obj-fx-trunk/browser/app' gmake[4]: *** [export] Error 2 gmake[4]: Leaving directory `/builds/tinderbox/Fx-Trunk/Linux_2.6.18-53.1.13.el5_Depend/mozilla/obj-fx-trunk/browser' gmake[3]: *** [export_tier_app] Error 2 gmake[3]: Leaving directory `/builds/tinderbox/Fx-Trunk/Linux_2.6.18-53.1.13.el5_Depend/mozilla/obj-fx-trunk' gmake[2]: *** [tier_app] Error 2 gmake[2]: Leaving directory `/builds/tinderbox/Fx-Trunk/Linux_2.6.18-53.1.13.el5_Depend/mozilla/obj-fx-trunk' gmake[1]: *** [alldep] Error 2 gmake[1]: Leaving directory `/builds/tinderbox/Fx-Trunk/Linux_2.6.18-53.1.13.el5_Depend/mozilla/obj-fx-trunk' gmake: *** [alldep] Error 2

Comment on attachment 327877 [details] [diff] [review] application.ini + Makefile.in - Add source stamp to application.ini I suspect >+ifdef $(MOZ_CO_DATE) should be ifdef MOZ_CO_DATE

Sorry for that - I have fixed the line that Axel mentioned and it is baking on the try server

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1219354764.1219357885.18671.gz http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1219354764.1219357114.16539.gz http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1219354764.1219359205.22231.gz It build in all 3 platforms

Please checkin-needed for the patch

Checked in on FF3.0 branch: Checking in browser/app/Makefile.in; new revision: 1.159; previous revision: 1.158 Checking in browser/app/application.ini; new revision: 1.14; previous revision: 1.13

This landed in CVS but not in mercurial? Should we reopen this bug for mercurial or file a new one?

I recall that the idea was to add the changeset to application.ini for mercurial. Didn't find a bug on that, though.

ok, I filed bug 452426 for the changeset id