Closed Bug 407319 Opened 12 years ago Closed 12 years ago

application.ini should supply timestamp information

Categories

(Firefox Build System :: General, defect)

1.9.0 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9

People

(Reporter: rhelmer, Assigned: armenzg)

References

Details

(Keywords: fixed1.9.0.2)

Attachments

(1 file, 6 obsolete files)

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
Assignee: nobody → armenzg
Status: NEW → ASSIGNED
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
Attachment #326752 - Flags: review?(benjamin)
Attachment #326752 - Flags: review?(ted.mielczarek)
Attachment #326752 - Flags: review?(benjamin)
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?
Attachment #326751 - Attachment is obsolete: true
Attachment #326816 - Flags: review?(ted.mielczarek)
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
Attachment #326816 - Flags: review?(ted.mielczarek)
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?
Attachment #326752 - Attachment is obsolete: true
Attachment #326955 - Flags: review?(ted.mielczarek)
Attachment #326752 - Flags: review?(ted.mielczarek)
Attachment #326816 - Attachment is obsolete: true
Attachment #326956 - Flags: review?(ted.mielczarek)
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).
Attachment #326955 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #326956 - Flags: review?(ted.mielczarek) → review-
(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

Attachment #327877 - Flags: review?(ted.mielczarek)
Attachment #326955 - Attachment is obsolete: true
Attachment #326956 - Attachment is obsolete: true
Comment on attachment 327877 [details] [diff] [review]
application.ini + Makefile.in - Add source stamp to application.ini

Looks good.
Attachment #327877 - Flags: review?(ted.mielczarek) → review+
Blocks: 398954
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.
Attachment #327877 - Flags: approval1.9.0.2?
(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
Attachment #327877 - Flags: approval1.9.0.2? → approval1.9.0.2+
Keywords: checkin-needed
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.
Keywords: checkin-needed
(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


Keywords: checkin-needed
"trunk in CVS" == 1.9 *branch*. mozilla-central is known as trunk in development talk. Calling CVS HEAD the "trunk" is far too confusing.
Whiteboard: [needs checkin on the 1.9 branch]
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs checkin on the 1.9 branch]
Target Milestone: --- → mozilla1.9
I've backed this out since it made linux and windows build machines fail.
Status: RESOLVED → REOPENED
Keywords: fixed1.9.0.2
Resolution: FIXED → ---
Target Milestone: mozilla1.9 → ---
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
Attachment #327877 - Attachment is obsolete: true
Attachment #334956 - Flags: review?(ted.mielczarek)
Attachment #334956 - Flags: review?(ted.mielczarek)
Please checkin-needed for the patch
Keywords: checkin-needed
Summary: application.ini should supply branch/timestamp information → application.ini should supply timestamp information
Whiteboard: [needs checkin on 1.9.0]
Version: unspecified → 1.9.0 Branch
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [needs checkin on 1.9.0]
Target Milestone: --- → mozilla1.9
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.