The default bug view has changed. See this FAQ.

toolkit/xre clean target messages

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: joey, Assigned: joey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Updated

6 years ago
Assignee: nobody → joey
(Assignee)

Comment 1

5 years ago
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.
Attachment #605731 - Flags: review?(khuey)
(Assignee)

Comment 2

5 years ago
ping on the code review
(Assignee)

Comment 3

5 years ago
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.
Attachment #605731 - Attachment is obsolete: true
Attachment #605731 - Flags: review?(khuey)
Attachment #606640 - Flags: review?(khuey)
(Assignee)

Comment 4

5 years ago
ping on code review
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.
Attachment #606640 - Flags: review?(khuey) → review+
(Assignee)

Comment 6

5 years ago
(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.
I wouldn't bother writing an automated test for it.
(Assignee)

Comment 8

5 years ago
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
Attachment #606640 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Comment 10

5 years ago
try job: http://tbpl.mozilla.org/?tree=Try&rev=7fb97abc26f5
Comment on attachment 609722 [details] [diff] [review]
patch carried forward with 1 bugfix & error checking

https://hg.mozilla.org/projects/build-system/rev/106c7696d3b7
(Assignee)

Comment 12

5 years ago
Patch landed on mozilla-central yesterday by Coop & Philor:
https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 746151
You need to log in before you can comment on or make changes to this bug.