448155 - builds should have changeset ID in about:buildconfig when possible

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should really find a way to have builds -- at least the nightlies and hourlies and probably releases that we produce -- have the mercurial changeset ID in about:buildconfig so testers can have a better idea what they're testing.

Steps to reproduce: enter about:buildconfig in the URL bar
Expected results: mercurial changeset ID present
Actual results: mercurial changeset ID not present

(It's possible we might want this ifdef MOZILLA_OFFICIAL or something like that -- assuming that we use that for all nightlies/hourlies.)

Comment 1

[(not currently active) Ted Mielczarek]

Armen has a bug on putting the changeset ID into application.ini, for l10n repackaging. 

Comment 2

[Jim Blandy :jimb]

I'd like to see the changeset ID in about:buildconfig, as well.  It seems like the natural place to look for it.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: add a source link to about:buildconfig

This patch changes the way we create buildconfig.html. Instead of treating it like a Makefile and letting configure do the substitution, we run it through the preprocessor when it's put into a jar file. I had to add two variables that wouldn't otherwise be persisted this far to autoconf.mk, and add all the things it expects to DEFINES in the makefile, but otherwise it's pretty straightforward. Since this is a HTML page anyway, I made it create a link to the source changeset in hgweb.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: fix some minor issues

Forgot to remove buildconfig.dtd (a remnant of a previous approach), and fixed it so that repo URLs without a trailing slash will work. I'm just stripping off the trailing slash, and re-adding a slash in the html file.

Comment 5

[Benjamin Smedberg]

Comment on attachment 339771 [details] [diff] [review]
fix some minor issues

>diff --git a/toolkit/content/Makefile.in b/toolkit/content/Makefile.in

>-DEFINES += -DMOZ_APP_VERSION=$(MOZ_APP_VERSION)
>+DEFINES += \
>+  -DMOZ_APP_VERSION=$(MOZ_APP_VERSION) \
>+  -Dtarget="$(target)" \
>+  -Dac_configure_args="$(ac_configure_args)" \
>+  -DCC="$(CC)" \
>+  -DCC_VERSION="$(CC_VERSION)" \
>+  -DCFLAGS="$(CFLAGS)" \
>+  -DCXX="$(CXX)" \
>+  -DCXX_VERSION="$(CXX_VERSION)" \
>+  -DCXXFLAGS="$(CXXFLAGS)" \
>+  -DCPPFLAGS="$(CPPFLAGS)" \
>+  $(NULL)

I'm a little afraid of this requiring more shell escaping: I don't think it's unheard-of to do

CC='gcc "-g3"'

which would break your quoting. Perhaps a small makefile function could be used to escape these:

shellescape="$(subst ",\",$(subst \,\\,$(1)))"

-DCC=$(call shellescape,$(CC))

Comment 6

[(not currently active) Ted Mielczarek]

That syntax just seems to confuse configure or gcc:
checking for gcc... gcc "-ggdb3"

checking whether the C compiler (gcc "-ggdb3"  ) works... no

configure: error: installation or configuration problem: C compiler cannot create executables.

Is there some other similar syntax you're worried about?

Comment 7

[Benjamin Smedberg]

Comment on attachment 339771 [details] [diff] [review]
fix some minor issues

ok

Comment 8

[(not currently active) Ted Mielczarek]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/dedc9cf1052c

Comment 9

[Jeff Muizelaar [:jrmuizel]]

This change breaks building in scratchbox. The scratchbox hg doesn't support 'identifty -i'. The makefile should probably check the exit code of hg and do something appropriate if it fails.

Comment 10

[Benjamin Smedberg]

Come on, can you at least be more specific about the failure message or failure mode?

Comment 11

[(not currently active) Ted Mielczarek]

We discussed this in #developers, and I said I'd be ok with using `hg identify | cut -f1 -d' '`, since that appears to work in scratchbox.

The error code shouldn't matter, since the command redirects stderr to /dev/null, so it should just wind up empty. Unfortunately, hg appears to put its help text on stdout for some reason.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #10)
> Come on, can you at least be more specific about the failure message or failure
> mode?

hg outputs its help text to stdout which gets set as CHANGESET causing failures in Preprocess.py later on.

(In reply to comment #11)
> We discussed this in #developers, and I said I'd be ok with using `hg identify
> | cut -f1 -d' '`, since that appears to work in scratchbox.
> 
> The error code shouldn't matter, since the command redirects stderr to
> /dev/null, so it should just wind up empty. Unfortunately, hg appears to put
> its help text on stdout for some reason.

Doesn't it make more sense to check whether the command succeeded then whether it wrote any text to stdout like the code currently does?

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

The root cause being:
  [sbox-CHINOOK-ARMEL-2007: ~] > hg --version
  Mercurial Distributed SCM (version 0.9.1)

Comment 14

[Nick Thomas [:nthomas] (UTC+12)]

FTR, the linux slaves are otherwise using Mercurial 0.9.5 for the mozilla-central and tracemonkey builds.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fixup patch

Here's the fixup patch; was going to land this, but tree is closed.  Tested this on win32, works there as well.

Comment 16

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #10)
> Come on, can you at least be more specific about the failure message or failure
> mode?

Within scratchbox (which uses hg v0.9.1), there is no support for "hg identify -i...", so attempts to use "-i" will fail out, and generate help-usage text. No error is caught, so that help-usage text is then appended to -DSOURCE_CHANGESET, causing the build to fail with invalid parameters.

Even without rest of build complexity, it can be reproduced by doing:
scratchbox>hg identify -i .
hg identify: option -i not recognized
hg identify
print information about the working copy
    Print a short summary of the current state of the repo.
    This summary identifies the repository state using one or two parent
    hash identifiers, followed by a "+" if there are uncommitted changes
    in the working directory, followed by a list of tags for this revision.
aliases: id

However, the following works just fine:
scratchbox> hg identify | cut -f1 -d' '
667a63ed2a63
scratchbox>

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Also same thing in browser/app/Makefile.in... checked in to unbreak mobile builds.

Pushed:
19526[tip]   ccdb84fa5b6a   2008-09-22 17:30 -0700   vladimir
  b=448155; fixup patch for hg identify -i not working in scratchbox to fix mobile build bustage; r=me

Comment 18

[Benjamin Smedberg]

Wow, 0.9.1? We're going to have to upgrade that before we do names release branches!

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

And pushed again, this time with "cd $(srcdir) ; hg identify" instead of "hg identify $(srcdir)".  And yes, hg will need to be upgraded inside scratchbox for sure.

Comment 20

[(not currently active) Ted Mielczarek]

vlad: you shouldn't have to touch browser/app/Makefile, since AFAIK fennec doesn't build that.

Comment 21

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Vlad: thanks for landing that, the builds now gets past those two failures.

Ted: sadly, even after Vlad's bustage fix, the builds still fail. A few lines further down, it turns out that "hg -R" doesnt work either. :-(

toolkit/content/Makefile.in, line#70
SOURCE_REPO := $(shell hg -R $(topsrcdir) showconfig paths.default 2>/dev/null | sed -e "s/^ssh:/http:/" -e "s/\/$(_dollar)//" )

browser/app/Makefile.in, line#80
SOURCE_REPO := $(shell hg -R $(topsrcdir) showconfig paths.default 2>/dev/null | sed s/^ssh:/http:/)


These "hg -R" both fail out, throwing a hg usage error message, this means SOURCE_REPO contains invalid text, and subsequent build steps fail.

Help?

Comment 22

[(not currently active) Ted Mielczarek]

Attachment: [checked in] potential bustage fix

Here's a possible bustage fix. I pastebin'ed this to stuart, but I'm not sure he got to try it. Someone want to give this a whirl in scratchbox?

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 339957 [details] [diff] [review]
[checked in] potential bustage fix

Pushed (without that extraneous if):
http://hg.mozilla.org/mozilla-central/rev/31f1081d9681

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

If we still fail, we should probably just upgrade hg and back out those two changes -- we know we'll need an upgrade at some point anyway, as soon as we do a branch.