Last Comment Bug 688061 - toolkit/xre clean target messages
: toolkit/xre clean target messages
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: ---
Assigned To: Joey Armstrong [:joey]
:
:
Mentors:
Depends on:
Blocks: 746151
  Show dependency treegraph
 
Reported: 2011-09-20 16:46 PDT by Joey Armstrong [:joey]
Modified: 2012-04-17 07:53 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
toolkit/xre/Makefile.in - reduce shell command use (5.16 KB, patch)
2012-03-14 07:00 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
toolkit/xre/Makefile.in - reduce shell command use (4.02 KB, patch)
2012-03-16 11:08 PDT, Joey Armstrong [:joey]
khuey: review+
Details | Diff | Splinter Review
patch carried forward with 1 bugfix & error checking (4.71 KB, patch)
2012-03-27 08:36 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2011-09-20 16:46:47 PDT
Messages logged during a nop try job submission.  A modified version of rules.mk was submitted with a bogus makefile target inserted.

LOG: try-linuxqt-build578.txt.gz 

make -C xre clean
cat: ../../config/buildid: No such file or directory
make[5]: Entering directory `/builds/slave/try-linuxqt/build/obj-firefox/toolkit/xre'

#######################################################################
 Solaris /usr/bin/diff doesn't have -n option
GRE_MILESTONE := $(shell tail -n 1 $(topsrcdir)/config/milestone.txt 2>/dev/null || tail -1 $(topsrcdir)/config/milestone.txt)
GRE_BUILDID := $(shell cat $(DEPTH)/config/buildid)
#######################################################################

1) redirect stderr to /dev/null in the GRE_BUILDID macro.  If a non-existent buildid file is an error for all other non-clean targets a conditional can be added to test $(GRE_BUILDID) == $(NULL) and conditionally $(error ) out when detected.

2) GRE_MILESTONE logic can be simplified.  Replace tail/shell conditionals with:
   GRE_MILESTONE := $(lastword $(shell cat ..../milestone.txt))
Comment 1 Joey Armstrong [:joey] 2012-03-14 07:00:37 PDT
Created attachment 605731 [details] [diff] [review]
toolkit/xre/Makefile.in - reduce shell command use

SOURCE_REPO, GRE_MILESTONE, GRE_BUILDID - replace shell pipelines with a single command followed by make string manipulation.

Moved SOURCE_REPO= inside MOZ_SOURCE_STAMP conditional so commands will only run when needed.
Comment 2 Joey Armstrong [:joey] 2012-03-16 06:41:49 PDT
ping on the code review
Comment 3 Joey Armstrong [:joey] 2012-03-16 11:08:21 PDT
Created attachment 606640 [details] [diff] [review]
toolkit/xre/Makefile.in - reduce shell command use

Same patch contents as last time but reverted the license block for automated batch updates.
Comment 4 Joey Armstrong [:joey] 2012-03-26 05:33:57 PDT
ping on code review
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-26 08:14:03 PDT
Comment on attachment 606640 [details] [diff] [review]
toolkit/xre/Makefile.in - reduce shell command use

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

Assuming that about:buildconfig looks the same afterwards, r=me.
Comment 6 Joey Armstrong [:joey] 2012-03-27 06:14:34 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> Comment on attachment 606640 [details] [diff] [review]
> toolkit/xre/Makefile.in - reduce shell command use
> 
> Review of attachment 606640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Assuming that about:buildconfig looks the same afterwards, r=me.

Visually verifying about:buidconfig is easy enough.  Would it make sense to setup a Makefile.in unit test that will parse/load $(obj)/dist/bin/chrome/toolkit/content/global/buildconfig.html and verify values for GRE_MILESTONE, GRE_BUILDID, TOOLKIT_EM_VERSION, ... have been properly branded into the about content file ?

$(call errorIfEmpty,GRE_BUILDID) tests can be inserted into the makefile to check for missing values early { after bug 735368 lands } but there is not yet an automated test to verify they were able to propagate where needed.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-27 07:54:12 PDT
I wouldn't bother writing an automated test for it.
Comment 8 Joey Armstrong [:joey] 2012-03-27 08:36:51 PDT
Created attachment 609722 [details] [diff] [review]
patch carried forward with 1 bugfix & error checking

patsubst to massage hg repo URL should replace with content rather than $(NULL) when stripping a trailing slash.

Added calls to $(checkifEmpty), $(warnIfEmpty) to verify values gathered by shell commands were able to return a value.  Test will be a NOP until bug 735638 lands and provides a definition for make user func checkIfError.
r=khuey, r=me
Comment 9 Joey Armstrong [:joey] 2012-03-27 08:38:29 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> Comment on attachment 606640 [details] [diff] [review]
> toolkit/xre/Makefile.in - reduce shell command use
> 
> Review of attachment 606640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Assuming that about:buildconfig looks the same afterwards, r=me.

about::buildconfig looks the same and verified $(obj)/dist/bin/platform.ini contained correct values for source repo, Milestone and Buildid.
Comment 10 Joey Armstrong [:joey] 2012-03-28 07:29:22 PDT
try job: http://tbpl.mozilla.org/?tree=Try&rev=7fb97abc26f5
Comment 11 Chris Cooper [:coop] 2012-04-03 12:59:18 PDT
Comment on attachment 609722 [details] [diff] [review]
patch carried forward with 1 bugfix & error checking

https://hg.mozilla.org/projects/build-system/rev/106c7696d3b7
Comment 12 Joey Armstrong [:joey] 2012-04-05 10:11:30 PDT
Patch landed on mozilla-central yesterday by Coop & Philor:
https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7

Note You need to log in before you can comment on or make changes to this bug.