991237 - Add a --version flag to the shell to print out configuration info

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: show_version_info-v0.diff

Sample output:
-> ./gAd.tbpl.i/js/src/js --version
SpiderMonkey 31.0a1
	Built On:    Apr  2 2014 @ 11:53:41
	Built By:    /usr/bin/ccache g++ -m32 version 4.8.2 20131212 (Red Hat 4.8.2-7)
	C++ Target:  i686
	JIT Target:  arm
	Configation:  --target=i686-linux-gnu --disable-ctypes --enable-arm-simulator --disable-optimize --enable-debug --enable-signmar --enable-stdcxx-compat --disable-ctypes --enable-trace-malloc --with-ccache=/usr/bin/ccache --disable-shared-js --enable-posix-nspr-emulation --without-intl-api

I'm hoping that the fuzzers could be convinced to attach this to every bug they submit to make reproing easier for us and to reduce their own workload when filing.

I could not figure out an easy way to attach the hg build revision either, which I would seriously love to have. No reason to hold things up for that though.

I tested this with a number of configurations on linux under g++ and clang. I think the #else case should work with visual studio, but I haven't tested yet.

https://tbpl.mozilla.org/?tree=Try&rev=f26fb0e4825f

Comment 1

[Christian Holler (:decoder)]

Fantastic idea :) You'll get an extra beer if you add the hg revision ;)

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Woohoo! And yes, having the hg revision will be really nice. Perhaps gps or glandium might have some ideas on how to extract that?

Quick question: How about custom environment variables?

Sometimes I have env variables set as:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar 

or:

CC="gcc -mfloat-abi=softfp -B/usr/lib/gcc/arm-linux-gnueabi/4.7" AR=ar CXX="g++ -mfloat-abi=softfp -B/usr/lib/gcc/arm-linux-gnueabi/4.7"

for ARM.

I think the CC and CXX flags are handled but not AR=ar?

(AR=ar is funky but it sometimes is needed to bypass build issues, see bug 915960)

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8400820 [details] [diff] [review]
show_version_info-v0.diff

Review of attachment 8400820 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but it's touching configure.in and these sorts of things can have subtle gotchas to them, so I'm requesting build peer review. (I don't think it's a problem with this patch. The followup to add the hg revision is going to be trickier.)

::: js/src/shell/js.cpp
@@ +4908,5 @@
> +#else
> +    const char *jit_target = TARGET_CPU;
> +#endif
> +    fprintf(gOutFile, "\tJIT Target:  %s\n", jit_target);
> +    fprintf(gOutFile, "\tConfigation: %s\n", AC_CONFIGURE_ARGS);

I think you meant "Conflagration:", or maybe "Castigation:".

Comment 4

[Steve Fink [:sfink] [:s:]]

As a followup, I'd like to see everything in getBuildConfiguration() output too.

Comment 5

[Terrence Cole [:terrence]]

Attachment: show_version_info-v1.diff

Great suggestions, everyone!

New output:
╰> ./cdD.tbpl.i/js/src/js --version
SpiderMonkey-31.0a1
===================
	Build Date:  Apr  2 2014 @ 14:41:48
	Build Vars:  CXX="/usr/bin/ccache clang++" CC="/usr/bin/ccache clang" AR="ar"
	CPP Version: 4.2.1 Compatible Clang 3.3 (tags/RELEASE_33/final)
	Config Rev:  163806b5f9e4 | Bug 991237 - Add a --version option to the spidermonkey shell; NOT_REVIEWED
	Config Args: --enable-optimize --enable-debug --enable-signmar --enable-stdcxx-compat --disable-ctypes --enable-trace-malloc --with-ccache=/usr/bin/ccache --disable-shared-js --enable-posix-nspr-emulation --without-intl-api
	C++ Target:  x86_64
	JIT Target:  x86_64


https://tbpl.mozilla.org/?tree=Try&rev=e63ce8b6442b

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 8400896 [details] [diff] [review]
show_version_info-v1.diff

D'oh. Midaired with the previous review. Forwarding r?.

Comment 7

[Terrence Cole [:terrence]]

Attachment: show_version_info-v2.diff

Turns out ARM defines a CC variable.

Comment 8

[Terrence Cole [:terrence]]

https://tbpl.mozilla.org/?tree=Try&rev=f6a03eee1d95

Comment 9

[Terrence Cole [:terrence]]

Finally a green try run:
https://tbpl.mozilla.org/?tree=Try&rev=8cc140bbad62

I had to tweak the way we compute version a bit. It now checks the relevant defines directly and falls back to <unknown> if we don't find one set.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> I had to tweak the way we compute version a bit. It now checks the relevant
> defines directly and falls back to <unknown> if we don't find one set.

Is there a patch for this updated version? I recall trying to compile this on Windows shell and possibly hitting some compilation failures, but would like to retry.

Comment 11

[Terrence Cole [:terrence]]

Attachment: show_version_info-v3.diff

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> > I had to tweak the way we compute version a bit. It now checks the relevant
> > defines directly and falls back to <unknown> if we don't find one set.
> 
> Is there a patch for this updated version? I recall trying to compile this
> on Windows shell and possibly hitting some compilation failures, but would
> like to retry.

Yes, sorry, I forgot to upload after I finally got a green try run:

https://tbpl.mozilla.org/?tree=Try&rev=8cc140bbad62

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8402930 [details] [diff] [review]
show_version_info-v3.diff

Review of attachment 8402930 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +3982,5 @@
>  AC_SUBST(HOST_BIN_SUFFIX)
>  AC_SUBST(HOST_OS_ARCH)
>  
>  AC_SUBST(TARGET_CPU)
> +AC_DEFINE_UNQUOTED(TARGET_CPU, "$TARGET_CPU")

I'd rather see this and the other AC_DEFINES defined in moz.build, since the values are available there, instead of adding things to js-config.h.

@@ +4106,5 @@
>  AC_SUBST(ac_configure_args)
> +CONFIGURE_ARGS="$ac_configure_args"
> +AC_DEFINE_UNQUOTED(CONFIGURE_ARGS, "$CONFIGURE_ARGS")
> +
> +HG_REVISION=`hg log -r . --template "{node|short} | {desc}" || echo "<unknown>"`

I don't think {desc} should be there. Also, maybe it's time to put that in some build/autoconf/*.m4 file and make it available to both js and top-level, instead of the several different places in the source code that compute MOZ_SOURCE_STAMP

::: js/src/shell/js.cpp
@@ +4913,5 @@
> +#endif
> +
> +    fprintf(gOutFile, "SpiderMonkey-%s\n", MOZILLA_VERSION);
> +    fprintf(gOutFile, "===================\n");
> +    fprintf(gOutFile, "\tBuild Date:  %s @ %s\n", __DATE__, __TIME__);

Is there really value in this?

@@ +4915,5 @@
> +    fprintf(gOutFile, "SpiderMonkey-%s\n", MOZILLA_VERSION);
> +    fprintf(gOutFile, "===================\n");
> +    fprintf(gOutFile, "\tBuild Date:  %s @ %s\n", __DATE__, __TIME__);
> +    fprintf(gOutFile, "\tBuild Vars:  CXX=\"%s\" CC=\"%s\" AR=\"%s\"\n",
> +            CXX_ENVIRONMENT, CC_ENVIRONMENT, AR_ENVIRONMENT);

Is there really value in this?

@@ +4920,5 @@
> +    fprintf(gOutFile, "\tCPP Version: %s\n", version);
> +    fprintf(gOutFile, "\tConfig Rev:  %s\n", HG_REVISION);
> +    fprintf(gOutFile, "\tConfig Args:%s\n", CONFIGURE_ARGS); // Note: always has a leading space.
> +    fprintf(gOutFile, "\tC++ Target:  %s\n", TARGET_CPU);
> +    fprintf(gOutFile, "\tJIT Target:  %s\n", jit_target);

I'd just write out the JIT target if it's different.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> ::: js/src/shell/js.cpp
> @@ +4913,5 @@
> > +#endif
> > +
> > +    fprintf(gOutFile, "SpiderMonkey-%s\n", MOZILLA_VERSION);
> > +    fprintf(gOutFile, "===================\n");
> > +    fprintf(gOutFile, "\tBuild Date:  %s @ %s\n", __DATE__, __TIME__);
> 
> Is there really value in this?
> 
> @@ +4915,5 @@
> > +    fprintf(gOutFile, "SpiderMonkey-%s\n", MOZILLA_VERSION);
> > +    fprintf(gOutFile, "===================\n");
> > +    fprintf(gOutFile, "\tBuild Date:  %s @ %s\n", __DATE__, __TIME__);
> > +    fprintf(gOutFile, "\tBuild Vars:  CXX=\"%s\" CC=\"%s\" AR=\"%s\"\n",
> > +            CXX_ENVIRONMENT, CC_ENVIRONMENT, AR_ENVIRONMENT);
> 
> Is there really value in this?

Yes, these are useful for quick references regarding the shell, esp the date.

For the environment variables, it will be for when the developer would like to reproduce the configure parameters when recompiling a required shell, some of which had custom env variables.

Comment 14

[Mike Hommey [:glandium]]

The date and time are kind of irrelevant. You could be building today a changeset from two years ago.

Another concern I have is that date, time and mercurial revision essentially make compilation cache useless for that file.
Also, on further consideration, the mercurial revision is not very helpful for local builds because it doesn't necessarily match anything for other people. It also doesn't necessarily match anything locally, if you e.g. hg qref. Or if you just edit a file without making hg aware of that. If the interest is for the shell builds on the ftp, the mercurial revision should be available next to them or in the path to the file. If not, a file containing the mercurial revision could be added to the js archive.

Comment 15

[Terrence Cole [:terrence]]

(In reply to Mike Hommey [:glandium] from comment #14)
> The date and time are kind of irrelevant. You could be building today a
> changeset from two years ago.

The changeset revision is a bit opaque and has other issues (see below). My goal here is that new contributors filing a bug will notice that the version they are using is 2 years old and maybe retry on a newer version before filing. Another thing that happens occasionally is that someone will show up in #jsapi with an environment and toolchain that is more historical (or worse, /historical fiction/) than we are used to around here: e.g. ancient gcc, built 10 minutes ago is a different response than someone just having an ancient build.

> Another concern I have is that date, time and mercurial revision essentially
> make compilation cache useless for that file.

Good point, I think if we put these values in a separate, trivial compilation unit that should make the rebuilds pretty much irrelevant.

> Also, on further consideration, the mercurial revision is not very helpful
> for local builds because it doesn't necessarily match anything for other
> people.

Hence why I included {desc}, which will hopefully contain a Bug# or some other useful information.

> It also doesn't necessarily match anything locally, if you e.g. hg
> qref. Or if you just edit a file without making hg aware of that. If the
> interest is for the shell builds on the ftp, the mercurial revision should
> be available next to them or in the path to the file. If not, a file
> containing the mercurial revision could be added to the js archive.

Actually, even if you make hg aware of it, you still won't get anything new unless you reconfigure too. That is why I called it "Config Rev", not that it helps much. If you can think of any clever way to extract the rev at build time rather than configure time (without needing constant reconfigures), I'd love to hear it! Otherwise I think this is probably a "perfect is the enemy of good" situation: our main use case is for the fuzzers and this won't be an issue for them.

(In reply to Mike Hommey [:glandium] from comment #12)
> Comment on attachment 8402930 [details] [diff] [review]
> show_version_info-v3.diff
> 
> Review of attachment 8402930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/configure.in
> @@ +3982,5 @@
> >  AC_SUBST(HOST_BIN_SUFFIX)
> >  AC_SUBST(HOST_OS_ARCH)
> >  
> >  AC_SUBST(TARGET_CPU)
> > +AC_DEFINE_UNQUOTED(TARGET_CPU, "$TARGET_CPU")
> 
> I'd rather see this and the other AC_DEFINES defined in moz.build, since the
> values are available there, instead of adding things to js-config.h.

Neat! I didn't know that was possible.

> @@ +4915,5 @@
> > +    fprintf(gOutFile, "SpiderMonkey-%s\n", MOZILLA_VERSION);
> > +    fprintf(gOutFile, "===================\n");
> > +    fprintf(gOutFile, "\tBuild Date:  %s @ %s\n", __DATE__, __TIME__);
> > +    fprintf(gOutFile, "\tBuild Vars:  CXX=\"%s\" CC=\"%s\" AR=\"%s\"\n",
> > +            CXX_ENVIRONMENT, CC_ENVIRONMENT, AR_ENVIRONMENT);
> 
> Is there really value in this?

My ideal fuzz-bug workflow would be:
$ cd builddir
$ hg up <copy hg revision>
$ <copy env> ../configure <copy args>
$ ./js/src/js --version
<verify that things are more or less correct>

And if I can't repro locally, then I can at least hand the version info back and ask the fuzzers which difference is important.

> @@ +4920,5 @@
> > +    fprintf(gOutFile, "\tCPP Version: %s\n", version);
> > +    fprintf(gOutFile, "\tConfig Rev:  %s\n", HG_REVISION);
> > +    fprintf(gOutFile, "\tConfig Args:%s\n", CONFIGURE_ARGS); // Note: always has a leading space.
> > +    fprintf(gOutFile, "\tC++ Target:  %s\n", TARGET_CPU);
> > +    fprintf(gOutFile, "\tJIT Target:  %s\n", jit_target);
> 
> I'd just write out the JIT target if it's different.

Great idea!

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Terrence Cole [:terrence] from comment #11)
> Created attachment 8402930 [details] [diff] [review]
> show_version_info-v3.diff

Sample output from Windows:

fuzz1win@F1MINI ~/Desktop/js-dbg-32-dm-ts-windows-mozilla-central-690c810c8e3e-177692-991237-c11-d4f498c4128a-rqoh15
$ ./js-dbg-32-dm-ts-windows-690c810c8e3e-991237-c11-d4f498c4128a.exe --version
SpiderMonkey-31.0a1
===================
        Build Date:  Apr  9 2014 @ 15:13:50
        Build Vars:  CXX="cl" CC="cl" AR="lib"
        CPP Version: 160040219
        Config Rev:  <unknown>
        Config Args: --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-threadsafe '--with-nspr-prefix=c:\Users\fuzz1win\Desktop\shell-cache\js-dbg-32-dm-ts-windows-690c810c8e3e-991237-c11-d4f498c4128a\objdir-nspr-dbg-690c810c8e3e-8c8the\dist' '--with-nspr-cflags=-Ic:\Users\fuzz1win\Desktop\shell-cache\js-dbg-32-dm-ts-windows-690c810c8e3e-991237-c11-d4f498c4128a\objdir-nspr-dbg-690c810c8e3e-8c8the\dist\include\nspr' '--with-nspr-libs=c:Users♀uzz1winDesktopshell-cachejs-dbg-32-dm-ts-windows-690c810c8e3e-991237-c11-d4f498c4128aobjdir-nspr-dbg-690c810c8e3e-8c8thedistlib
spr4.lib c:Users♀uzz1winDesktopshell-cachejs-dbg-32-dm-ts-windows-690c810c8e3e-991237-c11-d4f498c4128aobjdir-nspr-dbg-690c810c8e3e-8c8thedistlibplds4.lib c:Users♀uzz1winDesktopshell-cachejs-dbg-32-dm-ts-windows-690c810c8e3e-991237-c11-d4f498c4128aobjdir-nspr-dbg-690c810c8e3e-8c8thedistlibplc4.lib'
        C++ Target:  i686
        JIT Target:  i686

Is the "<unknown>" config rev intended?

Also, note how the paths turn funky in "c:Users♀uzz1winDesktop", for example. (I'm guessing back vs forward slashes)

Comment 17

[Terrence Cole [:terrence]]

The path separator issue is... weird. No idea what's going on there.

The <unknown> is not intended either. It is coming from this line in configure:
  HG_REVISION=`hg log -r . --template "{node|short} | {desc}" || echo "<unknown>"`

So it seems that the command |hg log -r . --template "{node|short} | {desc}"| is failing. Could you try running it standalone to see what's failing?

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Terrence Cole [:terrence] from comment #17)
> So it seems that the command |hg log -r . --template "{node|short} |
> {desc}"| is failing. Could you try running it standalone to see what's
> failing?

Not sure if it's related but fyi my objdir is outside of the repository (perhaps I mentioned it before?), so the "." in `hg log -r . </snip>` would not refer to a valid repository.

Comment 19

[Terrence Cole [:terrence]]

Considering the amount of pushback I got from build peers for the basic concept, I'm loath to actually push forward with this.