688061 - toolkit/xre clean target messages

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

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]]

Attachment: 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]]

ping on the code review

Comment 3

[Joey Armstrong [:joey]]

Attachment: 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]]

ping on code review

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

I wouldn't bother writing an automated test for it.

Comment 8

[Joey Armstrong [:joey]]

Attachment: 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]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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]]

try job: http://tbpl.mozilla.org/?tree=Try&rev=7fb97abc26f5

Comment 11

[Chris Cooper [:coop] (he/him)]

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]]

Patch landed on mozilla-central yesterday by Coop & Philor:
https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7