Closed
Bug 948275
Opened 10 years ago
Closed 10 years ago
Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla29
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files, 4 obsolete files)
89.56 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
41.46 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
As being inherited from the first python implementation of config.status, ConfigEnvironment is not really nicely integrated with build backend stuff. Moreover, it assumes source directories and object directories are always equal, which doesn't work out for bug 778236.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8345120 -
Flags: review?(gps)
Assignee | ||
Comment 2•10 years ago
|
||
Forgot to add a comment I wanted to add about the CommonBackend stuff.
Attachment #8345123 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8345120 -
Attachment is obsolete: true
Attachment #8345120 -
Flags: review?(gps)
Assignee | ||
Comment 3•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2b2d902d9d9c
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8345639 -
Flags: review?(gps)
Assignee | ||
Comment 5•10 years ago
|
||
Rebased on top of the mozpack.path change.
Attachment #8345640 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8345123 -
Attachment is obsolete: true
Attachment #8345123 -
Flags: review?(gps)
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/try/rev/20a9f634e095
Assignee | ||
Comment 7•10 years ago
|
||
Note these patches are on top of https://bugzilla.mozilla.org/attachment.cgi?id=828531 which didn't land because in the end i didn't need it at the time. While that patch is not necessary here, it's necessary for bug 778236 anyways.
Comment 8•10 years ago
|
||
Comment on attachment 8345640 [details] [diff] [review] Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES Review of attachment 8345640 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/base.py @@ +294,5 @@ > + pp.context.update(self.environment.substs) > + pp.context.update(top_srcdir = self.environment.topsrcdir) > + pp.context.update(srcdir = srcdir) > + pp.context.update(relativesrcdir = mozpath.relpath(srcdir, self.environment.topsrcdir) or '.') > + pp.context.update(DEPTH = mozpath.relpath(self.environment.topobjdir, mozpath.dirname(obj.output_path)) or '.') Nit: cuddle = Can we just call update() with multiple arguments? That seems like it would be easier to read. ::: python/mozbuild/mozbuild/backend/common.py @@ +106,5 @@ > + if mozpath.basename(obj.output_path) == 'Makefile': > + return > + with self._get_preprocessor(obj) as pp: > + pp.do_include(obj.input_path) > + self.backend_input_files.add(obj.input_path) File a bug to capture included files in backend_input_files and depend it on bug 935987. @@ +159,5 @@ > + + ' ' \ > + + str(self.environment.defines[name]) \ > + + l[m.end('name'):] > + elif cmd == 'undef': > + l = '/* ' + l[:m.end('name')] + ' */' + l[m.end('name'):] Holy indentation, batman! Can you do "early return" instead? if not m: continue ... if not name: continue ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +1158,5 @@ > + # with a backslash. > + pp.handleLine(b'\n') > + pp.handleLine(b'include $(topsrcdir)/config/recurse.mk\n') > + if not stub: > + self.backend_input_files.add(obj.input_path) Please copy the comment about why this is necessary. ::: python/mozbuild/mozbuild/test/backend/test_configenvironment.py @@ -316,5 @@ > - self.assertEqual(env.get_input('/absolute/build/dir/file'), os.path.join(absolute, 'dir', 'file.in')) > - self.assertEqual(env.get_top_srcdir('/absolute/build/file'), '/some/path') > - self.assertEqual(env.get_top_srcdir('/absolute/build/dir/file'), '/some/path') > - self.assertEqual(env.get_file_srcdir('/absolute/build/file'), '/some/path') > - self.assertEqual(env.get_file_srcdir('/absolute/build/dir/file'), '/some/path/dir') The number of deleted tests in this file is somewhat unnerving. Feels like a regression in test coverage.
Attachment #8345640 -
Flags: review?(gps) → review+
Assignee | ||
Comment 9•10 years ago
|
||
I think this one is good.
Attachment #8345670 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8345639 -
Attachment is obsolete: true
Attachment #8345639 -
Flags: review?(gps)
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5efc079f3c93
Assignee | ||
Updated•10 years ago
|
Attachment #8345640 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Discussed over the air, but for posterity: (In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #8) > ::: python/mozbuild/mozbuild/backend/common.py > @@ +106,5 @@ > > + if mozpath.basename(obj.output_path) == 'Makefile': > > + return > > + with self._get_preprocessor(obj) as pp: > > + pp.do_include(obj.input_path) > > + self.backend_input_files.add(obj.input_path) > > File a bug to capture included files in backend_input_files and depend it on > bug 935987. Those preprocessed file only receive substitutions, not #includes or whatever else involving a marker. So there aren't any included files to capture :) > @@ +159,5 @@ > > + + ' ' \ > > + + str(self.environment.defines[name]) \ > > + + l[m.end('name'):] > > + elif cmd == 'undef': > > + l = '/* ' + l[:m.end('name')] + ' */' + l[m.end('name'):] > > Holy indentation, batman! > > Can you do "early return" instead? > > if not m: > continue > > ... > > if not name: > continue Sadly, there's a fh.write(l) at the end of the loop that needs to happen for each line. > ::: python/mozbuild/mozbuild/test/backend/test_configenvironment.py > @@ -316,5 @@ > > - self.assertEqual(env.get_input('/absolute/build/dir/file'), os.path.join(absolute, 'dir', 'file.in')) > > - self.assertEqual(env.get_top_srcdir('/absolute/build/file'), '/some/path') > > - self.assertEqual(env.get_top_srcdir('/absolute/build/dir/file'), '/some/path') > > - self.assertEqual(env.get_file_srcdir('/absolute/build/file'), '/some/path') > > - self.assertEqual(env.get_file_srcdir('/absolute/build/dir/file'), '/some/path/dir') > > The number of deleted tests in this file is somewhat unnerving. Feels like a > regression in test coverage. They are tests that are now irrelevant, so it's fine.
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8345672 [details] [diff] [review] Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES. Carrying over r+.
Attachment #8345672 -
Flags: review+
Comment 13•10 years ago
|
||
Comment on attachment 8345670 [details] [diff] [review] Use mozpack.path instead of os.path in mozbuild.frontend and mozbuild.backend modules Review of attachment 8345670 [details] [diff] [review]: ----------------------------------------------------------------- You just bit rotted like 10 patches :( ::: python/mozbuild/mozbuild/test/frontend/test_reader.py @@ +115,5 @@ > with self.assertRaises(BuildReaderError) as bre: > list(reader.read_topsrcdir()) > > e = bre.exception > + print e.actual_file !
Attachment #8345670 -
Flags: review?(gps) → review+
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d148626b568b https://hg.mozilla.org/integration/mozilla-inbound/rev/504eb03d80bc https://hg.mozilla.org/integration/mozilla-inbound/rev/64fdc0eed099
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #14) > https://hg.mozilla.org/integration/mozilla-inbound/rev/d148626b568b This is https://bugzilla.mozilla.org/attachment.cgi?id=828531
Assignee | ||
Comment 16•10 years ago
|
||
And a fixup to avoid clobber on windows https://hg.mozilla.org/integration/mozilla-inbound/rev/3381980228ed
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d148626b568b https://hg.mozilla.org/mozilla-central/rev/504eb03d80bc https://hg.mozilla.org/mozilla-central/rev/64fdc0eed099 https://hg.mozilla.org/mozilla-central/rev/3381980228ed
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 18•10 years ago
|
||
(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #8) > ::: python/mozbuild/mozbuild/test/backend/test_configenvironment.py > @@ -316,5 @@ > > - self.assertEqual(env.get_input('/absolute/build/dir/file'), os.path.join(absolute, 'dir', 'file.in')) > > - self.assertEqual(env.get_top_srcdir('/absolute/build/file'), '/some/path') > > - self.assertEqual(env.get_top_srcdir('/absolute/build/dir/file'), '/some/path') > > - self.assertEqual(env.get_file_srcdir('/absolute/build/file'), '/some/path') > > - self.assertEqual(env.get_file_srcdir('/absolute/build/dir/file'), '/some/path/dir') > > The number of deleted tests in this file is somewhat unnerving. Feels like a > regression in test coverage. Perhaps too much of a regression: these patches broke the comm-central build system horribly, by assuming that all of our moz.build files have a single topsrcdir and topobjdir.
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•