891766 - Add gecko's git commit info in B2G build

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Alan Huang]

Cureently, we cannot get gecko's git commit from B2G build.

If we get gecko from hg, we can have hg info included in buildconfig.html. But if we pull gecko from git server, we won't have similiar info included.

Hello Askeing,
I know Taiwan QA have been working on this. Can you help on this bug? Thanks.

Comment 1

[Askeing Yen[:askeing][:fyen]]

Attachment: bug-891766-fix.patch

Comment 2

[Askeing Yen[:askeing][:fyen]]

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

Comment 3

[Askeing Yen[:askeing][:fyen]]

Comment on attachment 773762 [details] [diff] [review]
bug-891766-fix.patch

Joey, could you help to review this patch?

Thank you.

Comment 4

[Joey Armstrong [:joey]]

Comment on attachment 773762 [details] [diff] [review]
bug-891766-fix.patch

Is this query something that must be run as part of *every* build attempt { shell commands are expensive on windows } ?  If value display will mainly be scriptable or interactive add/document another conditional variable so commands will only be run when needed.

## nit: unrelated whitespace change, vars should be left justified.
-DEFINES += -DSOURCE_CHANGESET="$(MOZ_SOURCE_STAMP)"
+  DEFINES += -DSOURCE_CHANGESET="$(MOZ_SOURCE_STAMP)"


 source_repo ?= $(call getSourceRepo)
 ifneq (,$(filter http%,$(source_repo)))
   DEFINES += -DSOURCE_REPO="$(source_repo)"
+else
+  MOZ_GIT_STAMP ?= $(shell cd $(topsrcdir) && git rev-parse --verify HEAD 2>/dev/null)

- Use a variable in place of inlined commands so they can be over-ridden/disabled from the command line.  Replace 'git' with $(GIT) and add a definition for GIT=git likely in configure.
- Use ifneq (,$(strip $(MOZ_GIT_STAMP))) in place of ifdef MOZ_GIT_STAMP.  ifdef would succeed even when the command fails.
- Could MOZ_GIT_STAMP be generalized so it might have a chance of being reused, maybe something like MOZ_CHANGESET_HEAD= ?
- Remove 2>/dev/null, redirecting can mask error conditions.
- Replace 'else' with a conditional to only run commands if in a git sandbox.  When mercurial fails on a getSourceRepo call or revision control is something other than git, shell commands will add overhead.

see config/makefiles/rcs.mk - MOZ_RCS_TYPE:
Also test for $(topsrcdir)/.git.
Prefer .hg whenever found to preserve existing functionality.
Add a conditional block for if $(findstring .git).
Define GIT ?= git then you can test for ifdef GIT in the makefile.

Comment 5

[Joey Armstrong [:joey]]

Comment on attachment 773762 [details] [diff] [review]
bug-891766-fix.patch

Is this query something that must be run as part of *every* build attempt { shell commands are expensive on windows } ?  If value display will mainly be scriptable or interactive add/document another conditional variable so commands will only be run when needed.

## nit: unrelated whitespace change, vars should be left justified.
-DEFINES += -DSOURCE_CHANGESET="$(MOZ_SOURCE_STAMP)"
+  DEFINES += -DSOURCE_CHANGESET="$(MOZ_SOURCE_STAMP)"


 source_repo ?= $(call getSourceRepo)
 ifneq (,$(filter http%,$(source_repo)))
   DEFINES += -DSOURCE_REPO="$(source_repo)"
+else
+  MOZ_GIT_STAMP ?= $(shell cd $(topsrcdir) && git rev-parse --verify HEAD 2>/dev/null)

- Use a variable in place of inlined commands so they can be over-ridden/disabled from the command line.  Replace 'git' with $(GIT) and add a definition for GIT=git likely in configure.
- Use ifneq (,$(strip $(MOZ_GIT_STAMP))) in place of ifdef MOZ_GIT_STAMP.  ifdef would succeed even when the command fails.
- Could MOZ_GIT_STAMP be generalized so it might have a chance of being reused, maybe something like MOZ_CHANGESET_HEAD= ?
- Remove 2>/dev/null, redirecting can mask error conditions.
- Replace 'else' with a conditional to only run commands if in a git sandbox.  When mercurial fails on a getSourceRepo call or revision control is something other than git, shell commands will add overhead.

see config/makefiles/rcs.mk - MOZ_RCS_TYPE:
Also test for $(topsrcdir)/.git.
Prefer .hg whenever found to preserve existing functionality.
Add a conditional block for if $(findstring .git).
Define GIT ?= git then you can test for ifdef GIT in the makefile.

Comment 6

[Askeing Yen[:askeing][:fyen]]

Attachment: bug-891766-fix.patch

(In reply to Joey Armstrong [:joey] from comment #5)
> Comment on attachment 773762 [details] [diff] [review]
> bug-891766-fix.patch
> 
> Is this query something that must be run as part of *every* build attempt {
> shell commands are expensive on windows } ?  If value display will mainly be
> scriptable or interactive add/document another conditional variable so
> commands will only be run when needed.
> 
> ## nit: unrelated whitespace change, vars should be left justified.
> -DEFINES += -DSOURCE_CHANGESET="$(MOZ_SOURCE_STAMP)"
> +  DEFINES += -DSOURCE_CHANGESET="$(MOZ_SOURCE_STAMP)"
Fixed

>  source_repo ?= $(call getSourceRepo)
>  ifneq (,$(filter http%,$(source_repo)))
>    DEFINES += -DSOURCE_REPO="$(source_repo)"
> +else
> +  MOZ_GIT_STAMP ?= $(shell cd $(topsrcdir) && git rev-parse --verify HEAD
> 2>/dev/null)
> 
> - Use a variable in place of inlined commands so they can be
> over-ridden/disabled from the command line.  Replace 'git' with $(GIT) and
> add a definition for GIT=git likely in configure.
Fixed

> - Use ifneq (,$(strip $(MOZ_GIT_STAMP))) in place of ifdef MOZ_GIT_STAMP. 
> ifdef would succeed even when the command fails.
Fixed

> - Could MOZ_GIT_STAMP be generalized so it might have a chance of being
> reused, maybe something like MOZ_CHANGESET_HEAD= ?
Rename to MOZ_CHANGESET_HEAD

> - Remove 2>/dev/null, redirecting can mask error conditions.
The config/makefiles/rcs.mk also use the 2>/dev/null to redirect the error message.
I afraid that if we remove it, the error message will assign to the MOZ_CHANGESET_HEAD.

> - Replace 'else' with a conditional to only run commands if in a git
> sandbox.  When mercurial fails on a getSourceRepo call or revision control
> is something other than git, shell commands will add overhead.
> 
> see config/makefiles/rcs.mk - MOZ_RCS_TYPE:
> Also test for $(topsrcdir)/.git.
> Prefer .hg whenever found to preserve existing functionality.
> Add a conditional block for if $(findstring .git).
> Define GIT ?= git then you can test for ifdef GIT in the makefile.
Fixed

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

Comment 7

[Askeing Yen[:askeing][:fyen]]

> > - Remove 2>/dev/null, redirecting can mask error conditions.
> The config/makefiles/rcs.mk also use the 2>/dev/null to redirect the error
> message.
> I afraid that if we remove it, the error message will assign to the
> MOZ_CHANGESET_HEAD.
MOZ_SOURCE_STAMP, not config/makefiles/rcs.mk.

Comment 8

[Joey Armstrong [:joey]]

(In reply to Askeing Yen[:askeing] from comment #7)
> > > - Remove 2>/dev/null, redirecting can mask error conditions.
> > The config/makefiles/rcs.mk also use the 2>/dev/null to redirect the error
> > message.
> > I afraid that if we remove it, the error message will assign to the
> > MOZ_CHANGESET_HEAD.
> MOZ_SOURCE_STAMP, not config/makefiles/rcs.mk.

Not a problem, var = $(shell ...) will be assigned only from stdin.
You would need to use var = $(shell $cmd 2>&1) to also collect stderr.

Here is an easy way to verify results:

MOZ_CHANGESET_HEAD = $(shell try-to-run-a-non-existent-command)
$(error MOZ_CHANGESET_HEAD=[$(MOZ_CHANGESET_HEAD)])

An error message should appear in your log output but not be part of the MOZ_CHANGESET assignment.



add $(error MOZ_CHANGESET_HEAD=[$(MOZ_CHANGESET_HEAD)])

Comment 9

[Askeing Yen[:askeing][:fyen]]

Attachment: bug-891766-fix.patch

(In reply to Joey Armstrong [:joey] from comment #8)
> (In reply to Askeing Yen[:askeing] from comment #7)
> Not a problem, var = $(shell ...) will be assigned only from stdin.
> You would need to use var = $(shell $cmd 2>&1) to also collect stderr.
> 
Fixed

Comment 10

[Joey Armstrong [:joey]]

Comment on attachment 776203 [details] [diff] [review]
bug-891766-fix.patch

Patch will need a build peer review to land.

Comment 11

[Askeing Yen[:askeing][:fyen]]

:joey could you tell me who can review this patch?
Thank you.

Comment 12

[Askeing Yen[:askeing][:fyen]]

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

Comment 13

[Askeing Yen[:askeing][:fyen]]

(In reply to Askeing Yen[:askeing] from comment #12)
> https://tbpl.mozilla.org/?tree=Try&rev=a095ee08bf3e

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

Comment 14

[Askeing Yen[:askeing][:fyen]]

Comment on attachment 776203 [details] [diff] [review]
bug-891766-fix.patch

Hi Michael,

Could you help to review it or assign to right person?

Thank you.

Comment 15

[Michael Wu [:mwu]]

gps, khuey, and glandium are some reviewers I can think of for this. Please check the blame or file logs for this code and guess an appropriate reviewer.

Comment 16

[Askeing Yen[:askeing][:fyen]]

Comment on attachment 776203 [details] [diff] [review]
bug-891766-fix.patch

Hi gps, could you please help to review?
Thanks.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 776203 [details] [diff] [review]
bug-891766-fix.patch

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

Please move the make logic that interacts with the version control systems to config/makefiles/rcs.mk.

Comment 18

[Askeing Yen[:askeing][:fyen]]

Attachment: bug-891766-fix.patch

Comment 19

[Askeing Yen[:askeing][:fyen]]

Comment on attachment 799320 [details] [diff] [review]
bug-891766-fix.patch

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

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 799320 [details] [diff] [review]
bug-891766-fix.patch

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

This looks good. It would be nice to print the URL of the Git repo to have parity with Mercurial. But, that can be a followup.

Comment 21

[Askeing Yen[:askeing][:fyen]]

Attachment: bug-891766-fix.patch

updated commit message

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5033980115fa

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5033980115fa

Comment 24

[Gregory Szorc [:gps]]

This broke my Mercurial build:

3:35.56 Traceback (most recent call last):
 3:35.56   File "/Users/gps/src/firefox/config/JarMaker.py", line 487, in <module>
 3:35.56     main()
 3:35.56   File "/Users/gps/src/firefox/config/JarMaker.py", line 484, in main
 3:35.57     jm.makeJar(infile, options.j)
 3:35.57   File "/Users/gps/src/firefox/config/JarMaker.py", line 228, in makeJar
 3:35.57     self.processJarSection(m.group('jarfile'), lines, jardir)
 3:35.57   File "/Users/gps/src/firefox/config/JarMaker.py", line 318, in processJarSection
 3:35.57     self._processEntryLine(m, outHelper, jf)
 3:35.57   File "/Users/gps/src/firefox/config/JarMaker.py", line 357, in _processEntryLine
 3:35.57     pp.do_include(inf)
 3:35.57   File "/Users/gps/src/firefox/config/Preprocessor.py", line 462, in do_include
 3:35.57     self.handleLine(l)
 3:35.57   File "/Users/gps/src/firefox/config/Preprocessor.py", line 241, in handleLine
 3:35.57     self.write(aLine)
 3:35.57   File "/Users/gps/src/firefox/config/Preprocessor.py", line 139, in write
 3:35.57     filteredLine = self.applyFilters(aLine)
 3:35.57   File "/Users/gps/src/firefox/config/Preprocessor.py", line 124, in applyFilters
 3:35.57     aLine = f[1](aLine)
 3:35.57   File "/Users/gps/src/firefox/config/Preprocessor.py", line 420, in filter_substitution
 3:35.57     return self.varsubst.sub(repl, aLine)
 3:35.57   File "/Users/gps/src/firefox/config/Preprocessor.py", line 418, in repl
 3:35.58     raise Preprocessor.Error(self, 'UNDEFINED_VAR', varname)
 3:35.58 Preprocessor.Error: ('/Users/gps/src/firefox/toolkit/content/buildconfig.html', 35, 'UNDEFINED_VAR', 'SOURCE_GIT_COMMIT')
 3:35.58 make[7]: *** [libs] Error 1
 3:35.58 make[6]: *** [content_libs] Error 2
 3:35.58 make[6]: *** Waiting for unfinished jobs....

Comment 25

[Askeing Yen[:askeing][:fyen]]

It works for me (both git and hg).

> #ifdef SOURCE_REPO
> #ifdef SOURCE_CHANGESET
> <h2>Source</h2>
> <p>Built from <a href="@SOURCE_REPO@/rev/@SOURCE_CHANGESET@">@SOURCE_REPO@/rev/@SOURCE_CHANGESET@</a></p>
> #endif
>+#else ifdef SOURCE_GIT_COMMIT
>+<h2>Source</h2>
>+<p>Built from git commit <a href="#">@SOURCE_GIT_COMMIT@</a></p>
> #endif
It's really wreid. If there is no SOURCE_GIT_COMMIT, then Line 33 should skip Line 34~35.

Would it be better if we modify the patch to following lines?
  #ifdef SOURCE_REPO
  #ifdef SOURCE_CHANGESET
  <h2>Source</h2>
  <p>Built from <a href="@SOURCE_REPO@/rev/@SOURCE_CHANGESET@">@SOURCE_REPO@/rev/@SOURCE_CHANGESET@</a></p>
  #endif
 +#else
 +#ifdef SOURCE_GIT_COMMIT
 +<h2>Source</h2>
 +<p>Built from git commit <a href="#">@SOURCE_GIT_COMMIT@</a></p>
 +#endif
  #endif

Comment 26

[Alan Huang]

I can build successfully for b2g and browser, both are clean build.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Askeing Yen[:askeing] from comment #25)
> It works for me (both git and hg).
> 
> > #ifdef SOURCE_REPO
> > #ifdef SOURCE_CHANGESET
> > <h2>Source</h2>
> > <p>Built from <a href="@SOURCE_REPO@/rev/@SOURCE_CHANGESET@">@SOURCE_REPO@/rev/@SOURCE_CHANGESET@</a></p>
> > #endif
> >+#else ifdef SOURCE_GIT_COMMIT
> >+<h2>Source</h2>
> >+<p>Built from git commit <a href="#">@SOURCE_GIT_COMMIT@</a></p>
> > #endif
> It's really wreid. If there is no SOURCE_GIT_COMMIT, then Line 33 should
> skip Line 34~35.
> 
> Would it be better if we modify the patch to following lines?
>   #ifdef SOURCE_REPO
>   #ifdef SOURCE_CHANGESET
>   <h2>Source</h2>
>   <p>Built from <a
> href="@SOURCE_REPO@/rev/@SOURCE_CHANGESET@">@SOURCE_REPO@/rev/
> @SOURCE_CHANGESET@</a></p>
>   #endif
>  +#else
>  +#ifdef SOURCE_GIT_COMMIT
>  +<h2>Source</h2>
>  +<p>Built from git commit <a href="#">@SOURCE_GIT_COMMIT@</a></p>
>  +#endif
>   #endif

That works. #elifdef SOURCE_GIT_COMMIT works too.

Comment 28

[William Chen [:wchen]]

This broke my git build on windows with the same error as gps. Ms2ger's suggestion fixed the problem for me.

Comment 29

[Askeing Yen[:askeing][:fyen]]

Attachment: bug-891766-fix-2.patch

gps and wchen, could you please help to test with this patch again?
thank you.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 801374 [details] [diff] [review]
bug-891766-fix-2.patch

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eaaa2592938

Comment 31

[Daniel Holbert [:dholbert]]

The first line of the commit message in Comment 30 still had "r=?", so I backed out and re-landed after updating that to r=gps (and deleting the then-duplicate second-line of the commit message).

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6318aa6aa84a
Re-land: https://hg.mozilla.org/integration/mozilla-inbound/rev/a38cf622cdc8

Comment 32

[Gregory Szorc [:gps]]

Filed bug 914269 to track the apparent qimportbz bug.

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a38cf622cdc8

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/1eaaa2592938