578906 - Don't call hg directly in the makefiles

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect, P4)

Description

[John Ford [:jhford] CET/CEST Berlin Time]

In the make files, we call mercurial directly with 'hg'.  Some examples are in http://mxr.mozilla.org/mozilla-central/search?string=hg&find=\.in&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Since mercurial is a program with a complicated set of dependencies it would be great if we could have a variable like we do for other tools.

I would write the patch myself except that I am not sure how to wire up the autoconf stuff to set $(HG) properly.

Comment 1

[(not currently active) Ted Mielczarek]

You just need three things:
1) A call to MOZ_PATH_PROGS(HG, hg) in configure.in to find the path
2) A call to AC_SUBST(HG) in configure.in to subst the path into the makefiles
3) A line like HG = @HG@ in config/autoconf.mk to receive the substitution and make it available in the build system

Then you can just go around and replace hg with $(HG) in makefiles.

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

I am going to take a brief look at this.  I have created two scripts in my path to log that hg is called properly for all builds that we do

[jhford@singe bug578906-hg-makefiles]$ cat ~/bin/hg
#!/bin/bash

echo FAIL: "$@" >> ~/hg-calls
/usr/bin/hg "$@"
[jhford@singe bug578906-hg-makefiles]$ cat ~/bin/hgfixed 
#!/bin/bash

echo PASS: "$@" >> ~/hg-calls
/usr/bin/hg "$@"

Comment 3

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: use autoconf to figure out which 'hg' to use instead of

pushed to try to be extra sure that I don't break anything, will r? when that gets back

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: fix typo in patch

gah, i have been working on specfiles a lot lately and accidentally used % instead of $ in a makefile :(

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 518580 [details] [diff] [review]
fix typo in patch

Skimming this I have some nits, I'm not a Core::build peer though, officially.

First, largest concern without looking deeply, Try *may* not catch every case of this (though it certainly catches the worst), is everywhere you change to $(HG) needs to be used after an include of autoconf.mk (or config.mk or rules.mk which include autoconf.mk for you) otherwise HG is empty.


>diff --git a/config/autoconf.mk.in b/config/autoconf.mk.in
> XARGS		= @XARGS@
>+HG			= @HG@
> STRIP		= @STRIP@

I don't know what your tab width is locally, but here it appears a 2-tab instead of 3-tab will actually make it line up.

>diff --git a/configure.in b/configure.in
>+dnl =======================================================
>+dnl = VCS options
>+dnl =======================================================
>+
>+MOZ_ARG_WITH_STRING(mercurial,
>+[ --with-mercurial=BIN location of mercurial script],
>+HG=$withval,
>+HG=$HG
>+)
>+if test -z "$HG" ; then
>+    MOZ_PATH_PROG(HG, hg)
>+fi
>+AC_SUBST(HG)
>+

I'm worried that someone with HG set in the environ may not actually be a workable hg, even though |hg| is in the path, can you drop the else part of MOZ_ARG_WITH_STRING, and instead use WITH_HG as the withval, and do MOZ_PATH_PROG(HG, $WITH_HG $HG hg) regardless of if test -z $HG, as that will find a correct |hg| that actually exists either way.

The alternative is not to let WITH_HG silently fail out, but I don't see how it would fail this early in configure if specified wrong anyway, you'd fail later on during the actual build if |--with-mercurial=/not/a/real/file|


>diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
> XARGS		= @XARGS@
>+HG			= @HG@
> STRIP		= @STRIP@

Same tab nit here.

>diff --git a/js/src/configure.in b/js/src/configure.in

If you change the above, change this to match.

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

I'm not sure that this bug is still valid and I suspect that it's not needed any longer.