Add gecko's git commit info in B2G build

RESOLVED FIXED

Status

Firefox OS
GonkIntegration
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Alan Huang, Assigned: askeing)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 773762 [details] [diff] [review]
bug-891766-fix.patch
Attachment #773762 - Flags: review?
(Assignee)

Comment 2

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=34e25a8193df
(Assignee)

Comment 3

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

Joey, could you help to review this patch?

Thank you.
Attachment #773762 - Flags: review? → review?(joey)

Updated

5 years ago
Attachment #773762 - Flags: review?(joey) → feedback?(jarmstrong)
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 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.
Attachment #773762 - Flags: feedback?(jarmstrong) → feedback-
(Assignee)

Comment 6

5 years ago
Created attachment 775503 [details] [diff] [review]
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
Attachment #773762 - Attachment is obsolete: true
Attachment #775503 - Flags: review?(joey)
(Assignee)

Comment 7

5 years ago
> > - 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.
(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)])
(Assignee)

Comment 9

5 years ago
Created attachment 776203 [details] [diff] [review]
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
Attachment #775503 - Attachment is obsolete: true
Attachment #775503 - Flags: review?(joey)
Attachment #776203 - Flags: review?(joey)
Comment on attachment 776203 [details] [diff] [review]
bug-891766-fix.patch

Patch will need a build peer review to land.
Attachment #776203 - Flags: review?(joey)
(Assignee)

Comment 11

5 years ago
:joey could you tell me who can review this patch?
Thank you.
(Assignee)

Comment 12

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=a095ee08bf3e
(Assignee)

Updated

5 years ago
Attachment #776203 - Flags: review?
(Assignee)

Comment 13

5 years ago
(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
(Assignee)

Comment 14

4 years ago
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.
Attachment #776203 - Flags: review? → review?(mwu)

Comment 15

4 years ago
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.

Updated

4 years ago
Attachment #776203 - Flags: review?(mwu)
(Assignee)

Comment 16

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

Hi gps, could you please help to review?
Thanks.
Attachment #776203 - Flags: review?(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.
Attachment #776203 - Flags: review?(gps)
(Assignee)

Comment 18

4 years ago
Created attachment 799320 [details] [diff] [review]
bug-891766-fix.patch
Attachment #776203 - Attachment is obsolete: true
(Assignee)

Comment 19

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

https://tbpl.mozilla.org/?tree=Try&rev=09e47ef96205
Attachment #799320 - Flags: review?(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.
Attachment #799320 - Flags: review?(gps) → review+
Assignee: nobody → fyen
(Assignee)

Comment 21

4 years ago
Created attachment 799998 [details] [diff] [review]
bug-891766-fix.patch

updated commit message
Attachment #799320 - Attachment is obsolete: true
Attachment #799998 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/5033980115fa
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5033980115fa
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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....
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

4 years ago
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
(Reporter)

Comment 26

4 years ago
I can build successfully for b2g and browser, both are clean build.
(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.
This broke my git build on windows with the same error as gps. Ms2ger's suggestion fixed the problem for me.
(Assignee)

Comment 29

4 years ago
Created attachment 801374 [details] [diff] [review]
bug-891766-fix-2.patch

gps and wchen, could you please help to test with this patch again?
thank you.
Attachment #801374 - Flags: review?(wchen)
Attachment #801374 - Flags: review?(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
Attachment #801374 - Flags: review?(wchen)
Attachment #801374 - Flags: review?(gps)
Attachment #801374 - Flags: review+
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
Filed bug 914269 to track the apparent qimportbz bug.
https://hg.mozilla.org/mozilla-central/rev/a38cf622cdc8
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/1eaaa2592938
You need to log in before you can comment on or make changes to this bug.