839298 - One True JSON file of build information

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Selena Deckelmann :selenamarie :selena]

Currently, crash-stats.mozilla.com figures out what our builds are for things like Firefox using this path and file: 

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-01-15-04-20-18-mozilla-aurora/firefox-20.0a2.en-US.linux-i686.txt

We parse out lots of metadata from the filename and path, and we use the date string in the file.

I would LOVE to move us to using a JSON file instead, that is a bit like this:

http://ftp.mozilla.org/pub/mozilla.org/b2g/manifests/1.0.0/2013-02-03-07/socorro_unagi_2013-02-03-07.json

The thing missing from that JSON file is 'platform'.

Comment 1

[Gregory Szorc [:gps]]

What you are saying is you want a machine-readable file with build metadata uploaded as part of building. That should be relatively easy to do since IIRC we have the ability to upload arbitrary files as part of one of the make targets. Which one I can't remember. package?

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

make package and make upload are team players in this.  I have a wip patch that fixes this.

Comment 3

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: generate + upload a json file

I've tested package and upload locally but haven't submitted to try yet.  This patch adds a small python script which takes paths to ini files and command line arguments.  The ini files are parsed using Python's library ini parser (ConfigParser).  In this version of the patch, the ini files are inserted into the json without substantially changing the structure.  I have another version which generates a flat dictionary which prefixes the keys with the section names of the file.  The command line arguments are in the form KEY=VALUE and inserted into the top level of the json dictionary.

I don't know which other build system variables or ini files would be handy for this, but adding them is trivial.

Comment 4

[Gregory Szorc [:gps]]

Perhaps writemozinfo.py could grow to meet the needs of RelEng?

Comment 5

[Jeff Hammel]

...almost worth CCing the whole tools team.  I'll probably have more to say later, but the general approach of having the build (+system, implied) information live with the builders and be broadcast/available to any consumer is IMHO the correct approach and not only solves any number of problems but will eliminate 1000s if not 10000s of lines of present and future code.

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Gregory Szorc [:gps] from comment #4)
> Perhaps writemozinfo.py could grow to meet the needs of RelEng?

This bug was filed by downstream consumers of releng's builds for the benefit of teams like QA and Socorro.

Oh, this patch should also have

diff -u b/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
--- b/toolkit/mozapps/installer/packager.mk
+++ b/toolkit/mozapps/installer/packager.mk
@@ -630,6 +630,9 @@
 		buildid=$(BUILDID) \
 		moz_source_repo=$(MOZ_SOURCE_REPO) \
 		moz_update_channel=$(MOZ_UPDATE_CHANNEL) \
+		moz_pkg_platform=$(MOZ_PKG_PLATFORM) \
+		target_os=$(TARGET_OS) \
+		target_cpu=$(TARGET_CPU) \
 		--ini $(DIST)/bin/application.ini \
 		--ini $(DIST)/bin/platform.ini \
 		$(NULL)

to include the platform, as requested in comment 0

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 712503 [details] [diff] [review]
generate + upload a json file

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

Drive-by: Instead of adding fields to packager.mk, I'd just do everything inside the Python script. If you |import buildconfig| you will have access to all the MOZ_* variables via buildconfig.subts, buildconfig.defines, etc. i.e. just have the packager |$(PYTHON) path/to/script > output/path|. Minimize LoC in make files.

::: toolkit/mozapps/installer/informulate.py
@@ +3,5 @@
> +import os
> +try:
> +    import json
> +except ImportError:
> +    import simplejson as json

Don't need this since we require 2.7 to build now.

Comment 8

[(not currently active) Ted Mielczarek]

writemozinfo.py is pretty hacky, but its purpose is to write out a JSON dict of specific build information for our test harnesses to use. It sounds like it might be similar in spirit to this, but I don't know if we'd want to merge them or not. (Also it should totally be rewritten to import buildconfig nowadays.)

Comment 9

[Jeff Hammel]

Yeah, fwiw I'm not convinced the mozinfo + writemozinfo.py solution is the best long-term fix (though it might be in some form part of the solution).  I suppose I generally break things in to different areas:

- you're a developer running in tree; in-tree software can point to information that lives in the build system.  This case is the easiest, though it would still be nice to get a summary in one file of all the stuff you care about in this area in one file

- you're a slave running tests; in this case the master knows (or at can at least help relay that information) information about the build information and the machine that sent it

- you're running tests or doing something like crashreporter where you need the data for firefox of some build out in the ether.  This is the case that is hard to solve.

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: using buildconfig to determine most of the variables needed

I used Greg's suggestion and used the buildconfig module to pick out variables.  BUILDID, MOZ_PKG_PLATFORM, MOZ_SOURCE_STAMP and MOZ_SOURCE_REPO are determined in packager.mk, so they need to be passed into the script through means other than buildconfig.

Some of these variables might not be useful, especially for universal builds.

Here is an example of output of a single arch mac 64-bit build.

$ cat obj-x86_64-apple-darwin11.4.2/dist/firefox-21.0a1.en-US.mac64.json 
{
  "as": "$(CC)", 
  "buildid": "20130211134036", 
  "cc": "/usr/bin/clang", 
  "cxx": "/usr/bin/clang++", 
  "host_alias": "x86_64-apple-darwin11.4.2", 
  "host_cpu": "x86_64", 
  "host_os": "darwin11.4.2", 
  "host_vendor": "apple", 
  "ld": "ld", 
  "moz_app_id": "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}", 
  "moz_app_maxversion": "21.0a1", 
  "moz_app_name": "firefox", 
  "moz_app_vendor": "Mozilla", 
  "moz_app_version": "21.0a1", 
  "moz_pkg_platform": "mac64", 
  "moz_source_repo": "https://hg.mozilla.org/mozilla-central", 
  "moz_source_stamp": "784b9beebe90", 
  "moz_update_channel": "default", 
  "target_alias": "x86_64-apple-darwin11.4.2", 
  "target_cpu": "x86_64", 
  "target_os": "darwin11.4.2", 
  "target_vendor": "apple"
}

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 712636 [details] [diff] [review]
using buildconfig to determine most of the variables needed

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

::: toolkit/mozapps/installer/informulate.py
@@ +1,2 @@
> +#!/usr/bin/env python
> +import sys

Needs an MPL2 header.

@@ +15,5 @@
> +            print "Malformed command line key value pairing"
> +            sys.exit(1)
> +        else:
> +            key, value = arg.split('=', 1)
> +            contents[key.lower()] = value

You might be able to write this a little nicer using partition:
http://docs.python.org/2/library/stdtypes.html#str.partition

@@ +22,5 @@
> +
> +def main():
> +    parser = optparse.OptionParser("%prog -- Generate build data in JSON format")
> +    parser.add_option("-o", "--output", dest="output", help="File to write output to")
> +    (options, args) = parser.parse_args()

I would probably just simplify this and make the first commandline argument the output file, and treat the rest as data.

@@ +35,5 @@
> +    important_substitutions.extend(['host_%s' % x for x in ('alias', 'cpu', 'os', 'vendor')])
> +    important_substitutions.extend(['MOZ_%s' % x for x in ('UPDATE_CHANNEL', 'APP_VENDOR',
> +                                              'APP_NAME', 'APP_VERSION', 'APP_MAXVERSION',
> +                                              'APP_ID')])
> +    important_substitutions.extend(['CC', 'CXX', 'LD', 'AS'])

Just move these up to the empty list in the first line?

::: toolkit/mozapps/installer/package-name.mk
@@ +65,4 @@
>  PKG_UPDATE_BASENAME = $(PKG_BASENAME)
>  CHECKSUMS_FILE_BASENAME = $(PKG_BASENAME)
>  MOZ_SOURCESTAMP_FILE_BASENAME = $(PKG_BASENAME)
> +MOZ_BUILDINFO_FILE_BASENAME = $(PKG_BASENAME)

Feels like we could probably compress these few variables into a single variable at this point.

::: toolkit/mozapps/installer/packager.mk
@@ +623,5 @@
>  	@echo "$(BUILDID)" > $(MOZ_SOURCESTAMP_FILE)
>  	@echo "$(MOZ_SOURCE_REPO)/rev/$(MOZ_SOURCE_STAMP)" >> $(MOZ_SOURCESTAMP_FILE)
>  
> +.PHONY: make-buildinfo-file
> +make-buildinfo-file::

Only needs one colon.

@@ +630,5 @@
> +		BUILDID=$(BUILDID) \
> +		MOZ_SOURCE_REPO=$(MOZ_SOURCE_REPO) \
> +		MOZ_SOURCE_STAMP=$(MOZ_SOURCE_STAMP) \
> +		MOZ_PKG_PLATFORM=$(MOZ_PKG_PLATFORM) \
> +		$(NULL)

The $(NULL) is a little weird in this context, since this is just a shell command. (It won't do anything, it's just weird looking.)

Comment 12

[Jeff Hammel]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> Comment on attachment 712636 [details] [diff] [review]
> using buildconfig to determine most of the variables needed
> 
> Review of attachment 712636 [details] [diff] [review]:

From the peanut gallery:

> @@ +15,5 @@
> > +            print "Malformed command line key value pairing"
> > +            sys.exit(1)
> > +        else:
> > +            key, value = arg.split('=', 1)
> > +            contents[key.lower()] = value
> 
> You might be able to write this a little nicer using partition:
> http://docs.python.org/2/library/stdtypes.html#str.partition

I never end up using str.partition.  Nothing against it, just never end up using it.

FWIW, I'd write this (and have written this) as

def parse_cmdline(args):
    """Take a list of strings in the format K=V and turn them into a python
    dictionary"""
    malformed = [arg for arg in args if '=' not in arg]:
    if malformed:
        print "Malformed command line 'key=value' pairing: %s" % ', '.join(malformed)
        sys.exit(1) # XXX a bit heavy-handed
    return dict([arg.split('=', 1) for arg in args])

(Probably better ways in 2.7 of doing this...i am recovering from a world of 2.4 darkness)

> @@ +35,5 @@
> > +    important_substitutions.extend(['host_%s' % x for x in ('alias', 'cpu', 'os', 'vendor')])
> > +    important_substitutions.extend(['MOZ_%s' % x for x in ('UPDATE_CHANNEL', 'APP_VENDOR',
> > +                                              'APP_NAME', 'APP_VERSION', 'APP_MAXVERSION',
> > +                                              'APP_ID')])
> > +    important_substitutions.extend(['CC', 'CXX', 'LD', 'AS'])
> 
> Just move these up to the empty list in the first line?

+1

Comment 13

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: version 2

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> Needs an MPL2 header.

Done!

> You might be able to write this a little nicer using partition:
> http://docs.python.org/2/library/stdtypes.html#str.partition

I haven't used partition before, good to know that it exists.
 
> I would probably just simplify this and make the first commandline argument
> the output file, and treat the rest as data.

Good idea!  Done.

> Just move these up to the empty list in the first line?

I expanded all of them and put them in a single list definition.  I was only using list comprehensions out of laziness.

> Feels like we could probably compress these few variables into a single
> variable at this point.

I renamed MOZ_SOURCESTAMP_FILE_BASENAME variable to MOZ_INFO_BASENAME, but I left the MOZ_SOURCESTAMP_FILE and MOZ_BUILDINFO_FILE definitions in package-name.mk, since that's where file names are determined.:w

> Only needs one colon.

Roger, fixed.  I noticed that the make-sourcestamp-file target also has two colons.  I've left it alone for now.

> The $(NULL) is a little weird in this context, since this is just a shell
> command. (It won't do anything, it's just weird looking.)

Urgh, I don't know why I did that.  I'm going to go ahead and blame it on not writing a gecko build system patch for a while.  Fixed


Since this patch was r+'d and I've addressed the nits, I'm going to land this tomorrow.

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 714191 [details] [diff] [review]
version 2

Carrying the r+ forward

https://hg.mozilla.org/integration/mozilla-inbound/rev/613836a25f92

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/613836a25f92

Comment 16

[Philip Chee]

I'm getting this error on make package:

c:\t1\hg\comm-central\mozilla\toolkit\mozapps\installer\packager.mk:622:0$ c:/t1/hg/objdir-sm/_virtualenv/Scripts/python.exe c:/t1/hg/comm-central/mozilla/config/nsinstall.py -D ../../mozilla/dist/
c:\t1\hg\comm-central\mozilla\toolkit\mozapps\installer\packager.mk:628:0$ c:/t1/hg/objdir-sm/_virtualenv/Scripts/python.exe c:/t1/hg/comm-central/mozilla/toolkit/mozapps/installer/informulate.py \
        ../../mozilla/dist//seamonkey-2.18a1.en-US.win32.json \
        BUILDID=20130217220900 \
        MOZ_SOURCE_REPO=read config from: c:\DEV\mozilla-build\hg\mercurial.ini \
        MOZ_SOURCE_STAMP=82c6a4c5dd37 \
        MOZ_PKG_PLATFORM=win32
Traceback (most recent call last):
  File "c:/t1/hg/comm-central/mozilla/toolkit/mozapps/installer/informulate.py", line 46, in <module>
    main()
  File "c:/t1/hg/comm-central/mozilla/toolkit/mozapps/installer/informulate.py", line 37, in main
    all_key_value_pairs = dict([(x.lower(), buildconfig.substs[x]) for x in important_substitutions])
KeyError: 'MOZ_APP_VENDOR'
c:\t1\hg\comm-central\mozilla\toolkit\mozapps\installer\packager.mk:628:0: command 'c:/t1/hg/objdir-sm/_virtualenv/Scripts/python.exe c:/t1/hg/comm-central/mozilla/toolkit/mozapps/installer/informulate.py \
        ../../mozilla/dist//seamonkey-2.18a1.en-US.win32.json \
        BUILDID=20130217220900 \
        MOZ_SOURCE_REPO=read config from: c:\DEV\mozilla-build\hg\mercurial.ini \
        MOZ_SOURCE_STAMP=82c6a4c5dd37 \
        MOZ_PKG_PLATFORM=win32' failed, return code 1

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

I haven't tested this yet, but I think this would fix your problem:

diff --git a/toolkit/mozapps/installer/informulate.py b/toolkit/mozapps/installer/informulate.py
--- a/toolkit/mozapps/installer/informulate.py
+++ b/toolkit/mozapps/installer/informulate.py
@@ -34,7 +34,7 @@ def main():
         'MOZ_APP_VERSION', 'MOZ_APP_MAXVERSION', 'MOZ_APP_ID',
         'CC', 'CXX', 'LD', 'AS']
 
-    all_key_value_pairs = dict([(x.lower(), buildconfig.substs[x]) for x in important_substitutions])
+    all_key_value_pairs = dict([(x.lower(), buildconfig.substs[x]) for x in important_substitutions if buildconfig.substs.has_key(x)])
     all_key_value_pairs.update(parse_cmdline(sys.argv[2:]))
 
     with open(sys.argv[1], "w+") as f:

Comment 18

[Philip Chee]

> I haven't tested this yet, but I think this would fix your problem:
Thanks that works for me but as Frank says in Bug 842254:

> But I think the
> real/correct(?) solution would be to make sure all the vars listed in
> http://hg.mozilla.org/mozilla-central/file/524e7bc67431/toolkit/mozapps/installer/informulate.py
> are included (probably not the right term) in the buildconfig python module. Not sure
> how exactly this works though...

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: b2g build errors

This patch fails B2G formal build from m-c git mirror. Both MOZ_SOURCE_REPO and MOZ_SOURCE_STAMP are empty because m-c source tree is not managed by hg. See attachment for detailed error messages.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: (follow-up): skip empty command line key value instead.

This patch is a simple work-around for errors shown in attachment #715408 [details]. I have no idea what's the json file meant for and this could result in incomplete or wrong content being generated.

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
> Created attachment 715412 [details] [diff] [review]
> (follow-up): skip empty command line key value instead.
> 
> This patch is a simple work-around for errors shown in attachment #715408 [details]
> [details]. I have no idea what's the json file meant for and this could
> result in incomplete or wrong content being generated.

This is information that is intended for the crash reporter team to parse.  Let's set the value to None instead of an empty string, but this seems otherwise great.

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 715412 [details] [diff] [review]
(follow-up): skip empty command line key value instead.

I'm not a build system peer, so I can't review this patch.  I think this change is OK, with a slight change.

>-            print "ERROR: Malformed command line key value pairing (%s)" % arg
>-            exit(1)
>+            print "WARNING: Malformed command line key value pairing (%s)" % arg
>+            continue

Because these variables are actually equal to the empty string in the build system, let's not bother with the 'continue' keyword and store the actual build system value of an empty string.

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to John Ford [:jhford] from comment #22)
> Because these variables are actually equal to the empty string in the build
> system, let's not bother with the 'continue' keyword and store the actual
> build system value of an empty string.

on second thought, 

> if value == '':

should be 

> if s == '':

since we don't care about the value being the empty string, we care about the separator not being present.

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: Only error out when the separator is invalid

This preserves the check for a valid key value pairing while allowing empty string values to be passed through.

Comment 25

[(not currently active) Ted Mielczarek]

Our official B2G builds use a hg clone of mozilla-central, so this information will be available there, right?

Comment 26

[John Ford [:jhford] CET/CEST Berlin Time]

The official builds use HG, but this does break git builds, which is what developers use.

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b969b60454

Mozilla-central is closed right now for merge day.

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to John Ford [:jhford] from comment #27)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b969b60454
> 
> Mozilla-central is closed right now for merge day.

It's opened up.

https://hg.mozilla.org/mozilla-central/rev/401b967b2dfc

Comment 29

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 715558 [details] [diff] [review]
Only error out when the separator is invalid

[Approval Request Comment]
Bug caused by (feature/regressing bug #): corner case (for branches) regression caused by this bug
User impact if declined: you cannot package a build if the source is not stored in an *HG* repository
Testing completed (on m-c, etc.): it works
Risk to taking this patch (and alternatives if risky): developers who use git or source snapshots or don't have mercurial installed will be completely unable to package their builds.  if we don't take this patch, then firefox-21 is slightly annoying to work on for the affected developers.
String or UUID changes made by this patch: none

Comment 30

[Justin Lebar (not reading bugmail)]

> if we don't take this patch, then firefox-21 is slightly annoying to work on for the 
> affected developers.

And additionally, many b2g developers will be unable to build this version of Gecko.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Alex Keybl [:akeybl]]

(In reply to John Ford [:jhford] from comment #29)
> Risk to taking this patch (and alternatives if risky): developers who use
> git or source snapshots or don't have mercurial installed will be completely
> unable to package their builds.  if we don't take this patch, then
> firefox-21 is slightly annoying to work on for the affected developers.

We're looking to approve this so that regression testing is unaffected (the only real use for Gecko 21 for b2g devs). Just want to confirm we expect no regressions to other products.

Comment 34

[Alex Keybl [:akeybl]]

jhford/ted (comment 32)? We're trying to make sure we aren't taking unnecessary/risky change here.

Comment 35

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Alex Keybl [:akeybl] from comment #34)
> jhford/ted (comment 32)? We're trying to make sure we aren't taking
> unnecessary/risky change here.

sorry, CC mail from uplifts has been doing a number on my bugmail mailbox.

There shouldn't be any regression to other products.

Comment 36

[(not currently active) Ted Mielczarek]

This is almost NPOTB. It generates a file as part of packaging and includes it in the upload to FTP, but that file has no impact on anything else. The fixup patch just unbreaks this for local B2G development.

Comment 37

[bhavana bajaj [:bajaj]]

Thanks :jhford, :ted ! From your comments the patch sounds safe , approving for uplift.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d230ceff8fc