515792 - build system runs identical 'hg identify' command too many times

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[David Baron :dbaron:]

'hg identify' can be quite slow:
 * because it does a full-tree diff
 * for other reasons, unclear to me, when a tree has a large number of heads
and we call it a significant number of times.  (I have a tree with a large number of heads in which it can take a significant part of a minute.)

We call an identical hg identify command from three different makefiles:
http://mxr.mozilla.org/mozilla-central/search?string=hg%20identi
for every phase of the build (export, libs, etc.(?)).

We should probably just call it once, either in configure (and put the output in a variable that gets AC_SUBSTed into autoconf.mk), or in the export phase of config/Makefile.in or something similar (and put the output in a file that can be cat-ed elsewhere).

Comment 1

[(not currently active) Ted Mielczarek]

I think we could also change the command slightly so it doesn't try to figure out if you have local changes. Something like:
hg identify -ir. ('.' is "the parent changeset of the working directory")

Comment 2

[David Baron :dbaron:]

You can also get that from:
hg parent --template="{node|short}\n"

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment #2 makes this sound like the easiest build-speed win available!

Comment 4

[Chris AtLee [:catlee]]

10 minute build times, here we come!

Comment 5

[:Gijs (he/him)]

Attachment: Patch to add comment #2 to configure.

Something like this?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

You could also AC_DEFINE it and then remove the manual addition to DEFINES in the two makefiles, I think.

Comment 7

[Axel Hecht [:Pike]]

Is running configure each time we update the tree really going to speed things up?

Comment 8

[:Gijs (he/him)]

Attachment: Patch

So the previous patch wouldn't have worked, I don't think (I mean, it built, but MOZ_SOURCE_STAMP was actually empty). This, on the other hand, does work (tested with about:buildconfig, which uses the variable). It adds MOZ_SOURCE_STAMP as an export in the toplevel makefile, and then in the sub-makefiles only assigns to that variable if it's not already defined. This ensures that the value is up-to-date even if make is only ran in subdirectories (thanks, Axel!).

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 410760 [details] [diff] [review]
Patch

+MOZ_SOURCE_STAMP = $(shell cd $(srcdir) && hg parent --template="{node|short}\n" 2>/dev/null)

Is this going to have the same problem that bug 517417 fixed, in that hg will look for a repo above the current directory?

Comment 10

[(not currently active) Ted Mielczarek]

Arne: could you test if the command in this patch (quoted in my previous comment) exhibits the same problem as bug 517417? I don't want to regress that for you.

Comment 11

[Arne Babenhauserheide]

Ted: Thank you for asking! 

It doesn't exibit the same problem, but it would lead to false information for your buildsystem (since it still finds the higher up hg repo). 

To make sure that the command bails out when it isn't in the top level directory of the repo, you can use 

hg parent --repository . --template="{node|short}\n"

But the cleanest way would probably be to use 

MOZ_SOURCE_STAMP ?= $(shell hg --repository $(topsrcdir) parent --template="{node|short}\n" 2>/dev/null)

This checks explicitely if $(topsrcdir) is a hg repository *root* and if yes it returns its parent. A shorthand would be "hg -R" instead of "hg --repository".

Comment 12

[(not currently active) Ted Mielczarek]

I think we might not have been using -R because we are trying to support some ancient version of hg (0.9.4?) that comes with Scratchbox, which our mobile developers use.

Comment 13

[Arne Babenhauserheide]

Could you test if -R works for them? 

For me hg 0.9.4 uses -R - I just tested it with 0.9, too.

Comment 14

[:Gijs (he/him)]

Attachment: Patch using -R flag

(In reply to comment #13)
> Could you test if -R works for them? 
> 
> For me hg 0.9.4 uses -R - I just tested it with 0.9, too.

This works for me in scratchbox, too:

[sbox-CHINOOK-ARMEL-2007: ~/devmoz/mozilla-central] > hg -R . parent --template="{node|short}\n" 2>/dev/null
001e14d17a30

Updated the patch using this approach instead.

Comment 15

[(not currently active) Ted Mielczarek]

Ok, I wasn't sure, I just remembered that that code went through several revisions, and we did have problems with the old hg in scratchbox. Thanks for testing!

Comment 16

[Arne Babenhauserheide]

Thank you for taking care!

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 413296 [details] [diff] [review]
Patch using -R flag

Looks good, thanks!

Comment 18

[:Gijs (he/him)]

http://hg.mozilla.org/mozilla-central/rev/fec4a5e4b36e

Comment 19

[Serge Gautherie (:sgautherie)]

Fwiw, should
{
/toolkit/crashreporter/tools/symbolstore.py
    * line 276 -- rev = os.popen('hg identify -i "%s"' % srcdir, "r").readlines()[0].rstrip()
}
be updated too?

Comment 20

[(not currently active) Ted Mielczarek]

No, not worth it.

Comment 21

[(not currently active) Ted Mielczarek]

And I don't see any reason we'd take this on 1.9.2.