Closed
Bug 663180
Opened 13 years ago
Closed 13 years ago
Make configure write mozinfo JSON data about build configuration for test harnesses to use
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: ted)
References
Details
(Whiteboard: fixed-in-bs)
Attachments
(1 file, 1 obsolete file)
10.69 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
configure should write out a JSON data file that test harnesses can pass to mozinfo to accurately reflect info about the current build configuration.
Assignee | ||
Comment 1•13 years ago
|
||
This is going to require us to bump PYTHON_VERSION to 2.6, I'll file that later. r?jhammel on the Python bits, and r?khuey on the build bits.
Attachment #538310 -
Flags: review?(khuey)
Attachment #538310 -
Flags: review?(jhammel)
Comment 2•13 years ago
|
||
Comment on attachment 538310 [details] [diff] [review] write $objdir/mozinfo.json during configure + def setUp(self): + self.tmpdir = mkdtemp() + self.f = os.path.join(self.tmpdir, "f") + + def tearDown(self): + rmtree(self.tmpdir) Since you only have one file, why make a temporary directory? Shouldn't a temporary file suffice? + with open(self.f, 'r') as f: The 'r' is implied + for r in required: + if r not in env: + raise Exception("Missing required environment variable '%s'" % r) You could report all of these at the same time, albeit there are only two: missing = [r for r in required if r not in env] if missing: raise Exception("Missing...: %s" % ', '.join(missing)) + if p in ["x86_64", "ppc64", "sparc64"]: + d["bits"] = 64 + else: + d["bits"] = 32 Is this dangerous? I guess not, mostly. I'd feel more comfortable if we only set to 32 for known values, but maybe its not worth the trouble. + with open(filename, "w") as f: While `with` is a better pattern, iirc it doesn't work with older python. I'd prefer to be as backwards-compatible as possible in case we need to backport +def write_json(filename, env=os.environ): <snip/> It might be nice to check if the filename is a file object, then just use that. Similarly + write_json(sys.argv[1]) could just use sys.stdout if len(sys.argv) == 1 +sys.path.append(os.path.join(os.path.dirname(__file__), '..')) Not a fan :( Overall, looks good! r+ I'd prefer if, for the simple values we have, we just kept a template (or templates) and populated that and dropped the json/python 2.6 dependency entirely.
Attachment #538310 -
Flags: review?(jhammel) → review+
Comment on attachment 538310 [details] [diff] [review] write $objdir/mozinfo.json during configure >diff --git a/configure.in b/configure.in >--- a/configure.in >+++ b/configure.in >@@ -9251,6 +9251,12 @@ > > AC_OUTPUT($MAKEFILES) > >+# Generate a JSON config file for unittest harnesses etc to read >+# build configuration details from in a standardized way. >+rm -f ./mozinfo.json >+export OS_TARGET TARGET_CPU MOZ_DEBUG >+$PYTHON ${_topsrcdir}/config/writemozinfo.py ./mozinfo.json >+ > dnl Prevent the regeneration of cairo-features.h forcing rebuilds of gfx stuff > if test "$CAIRO_FEATURES_H"; then > if cmp -s $CAIRO_FEATURES_H "$CAIRO_FEATURES_H".orig; then I'm not thrilled about exporting the variables. Can we pass them on the shell cmd? OS_TARGET=$OS_TARGET ... $PYTHON ... Also, if we're going to unconditionally rm -f mozinfo.json don't make anything depend on it in the build system!
Attachment #538310 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to comment #2) > + if p in ["x86_64", "ppc64", "sparc64"]: > + d["bits"] = 64 > + else: > + d["bits"] = 32 > Is this dangerous? I guess not, mostly. I'd feel more comfortable if > we only set to 32 for known values, but maybe its not worth the > trouble. Probably not, but I can make it a whitelist for both 32 and 64 so we'd just get unknown otherwise. > > + with open(filename, "w") as f: > > While `with` is a better pattern, iirc it doesn't work with older > python. I'd prefer to be as backwards-compatible as possible in case > we need to backport You can use with in 2.5, which we already require in the build system, so I don't think that's a problem. > +sys.path.append(os.path.join(os.path.dirname(__file__), '..')) > Not a fan :( Yeah, it's crappy, but it's just a unit test so I feel okay about it. > I'd prefer if, for the simple values we have, we just kept a template > (or templates) and populated that and dropped the json/python 2.6 dependency > entirely. Okay, I can do that, it's just slightly more of a pain. (In reply to comment #3) > I'm not thrilled about exporting the variables. Can we pass them on the > shell cmd? Yeah, I'll fix that. > Also, if we're going to unconditionally rm -f mozinfo.json don't make > anything depend on it in the build system! Right, I won't. If we need to, we can do the cmp trick like we do elsewhere to not modify it if it's unchanged.
Assignee | ||
Comment 5•13 years ago
|
||
I ditched the json module here in favor of some pretty simplistic Python -> JSON serialization for the small set of value types I care about. I kept the with statements, since you can import them into Python 2.5. We require 2.5 in the build system already, so there's no point in worrying about older backwards compat there. I added a few tests for some other things, I think I addressed all of your comments except for things I explicitly replied to above. jhammel: can you give this one more once-over?
Attachment #538310 -
Attachment is obsolete: true
Attachment #538538 -
Flags: review?(jhammel)
Assignee | ||
Comment 6•13 years ago
|
||
Also I went ahead and addressed khuey's comment, doing the right thing with cmp in configure so we don't overwrite the file when it's unchanged.
Comment 7•13 years ago
|
||
Comment on attachment 538538 [details] [diff] [review] write $objdir/mozinfo.json during configure, rev 2 looks great!
Attachment #538538 -
Flags: review?(jhammel) → review+
Assignee | ||
Comment 8•13 years ago
|
||
http://hg.mozilla.org/projects/build-system/rev/6d19baaa339f
Whiteboard: fixed-in-bs
Assignee | ||
Comment 9•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/6d19baaa339f
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•