Closed
Bug 1461836
Opened 6 years ago
Closed 6 years ago
Tup build invocation machinery should know when to run configure
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
Details
Attachments
(2 files)
One side effect of no longer invoking make in the tup build is that we no longer know when to run configure before building. I'm thinking we can do this along the lines of the backend rebuild checking code.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8976394 [details] Bug 1461836 - Write out complete configure dependencies from configure for consumption by make and non-make backends. https://reviewboard.mozilla.org/r/244544/#review250578 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: configure.py:17 (Diff revision 1) > > > base_dir = os.path.abspath(os.path.dirname(__file__)) > sys.path.insert(0, os.path.join(base_dir, 'python', 'mozbuild')) > from mozbuild.configure import ConfigureSandbox > from mozbuild.makeutil import Makefile Error: 'mozbuild.makeutil.makefile' imported but unused [flake8: F401]
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8976394 [details] Bug 1461836 - Write out complete configure dependencies from configure for consumption by make and non-make backends. https://reviewboard.mozilla.org/r/244544/#review250752 Overall I think this looks great, but I'd like to re-review after the issue is resolved. ::: client.mk:98 (Diff revision 1) > - $(wildcard $(TOPSRCDIR)/build/autoconf/*.m4) \ > - $(TOPSRCDIR)/js/src/aclocal.m4 \ > - $(TOPSRCDIR)/js/src/old-configure.in \ > - $(NULL) > > $(CONFIGURES): %: %.in $(EXTRA_CONFIG_DEPS) I don't think EXTRA_CONFIG_DEPS actually buys us anything anymore. It looks like this rule used to run autoconf, where presumably those files were read to generate files like 'configure' from 'configure.in'. But now that the rule just does 'cp configure.in configure', we don't need to re-run that if eg: aclocal.m4 changes. So I think we can merge EXTRA_CONFIG_DEPS into CONFIG_STATUS_DEPS and just run configure when CONFIG_STATUS_DEPS changes.
Attachment #8976394 -
Flags: review?(mshal)
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8976395 [details] Bug 1461836 - Check configure dependencies and re-run configure if needed when invoking the Tup backend. https://reviewboard.mozilla.org/r/244546/#review250758 ::: python/mozbuild/mozbuild/controller/building.py:973 (Diff revision 1) > disable_extra_make_dependencies=True > directory = mozpath.normsep(directory) > if directory.startswith('/'): > directory = directory[1:] > > - def backend_out_of_date(backend_file): > + def build_out_of_date(output, dep_files): With combined configure files, I think 'dep_files' here can be a string of a single dependency file instead of a list.
Attachment #8976395 -
Flags: review?(mshal)
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8976394 [details] Bug 1461836 - Write out complete configure dependencies from configure for consumption by make and non-make backends. https://reviewboard.mozilla.org/r/244544/#review250752 > I don't think EXTRA_CONFIG_DEPS actually buys us anything anymore. It looks like this rule used to run autoconf, where presumably those files were read to generate files like 'configure' from 'configure.in'. But now that the rule just does 'cp configure.in configure', we don't need to re-run that if eg: aclocal.m4 changes. > > So I think we can merge EXTRA_CONFIG_DEPS into CONFIG_STATUS_DEPS and just run configure when CONFIG_STATUS_DEPS changes. Yes, looks like it! Thanks for noticing that, will simplify this quite a bit.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8976394 [details] Bug 1461836 - Write out complete configure dependencies from configure for consumption by make and non-make backends. https://reviewboard.mozilla.org/r/244544/#review250876 ::: Makefile.in (Diff revision 2) > endif > > -$(topsrcdir)/configure: $(topsrcdir)/configure.in $(topsrcdir)/old-configure.in > -$(topsrcdir)/js/src/configure: $(topsrcdir)/js/src/configure.in $(topsrcdir)/js/src/old-configure.in > -$(topsrcdir)/configure $(topsrcdir)/js/src/configure: > - @echo 'STOP! $? has changed, and your configure is out of date.' Glad we can remove this! ::: configure.py:96 (Diff revision 2) > ''')) > > partial_config = PartialConfigEnvironment(config['TOPOBJDIR']) > partial_config.write_vars(sanitized_config) > > - # Write out a depfile so Make knows to re-run configure when relevant Python > + # Write out a file so Make knows to re-run configure when relevant Python nit: "Make" -> "mozbuild"? Or maybe "the build backend"? ::: moz.configure:579 (Diff revision 2) > -# Please do not add anything after setting ALL_CONFIGURE_PATHS. > + topobjdir = build_env.topobjdir > + > + return list(__sandbox__._all_paths) + [ > + os.path.join(topsrcdir, build_project, 'confvars.sh'), > + os.path.join(topsrcdir, 'configure'), > + os.path.join(topsrcdir, 'js', 'src', 'configure'), I think we also need configure.in and js/src/configure.in here. Prior to this patch, touching one of those files would re-run configure during 'mach build', but with this patch it no longer does.
Attachment #8976394 -
Flags: review?(mshal) → review+
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8976395 [details] Bug 1461836 - Check configure dependencies and re-run configure if needed when invoking the Tup backend. https://reviewboard.mozilla.org/r/244546/#review251058
Attachment #8976395 -
Flags: review?(mshal) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ec5cda49535f Write out complete configure dependencies from configure for consumption by make and non-make backends. r=mshal https://hg.mozilla.org/integration/autoland/rev/c7e9e54de140 Check configure dependencies and re-run configure if needed when invoking the Tup backend. r=mshal
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec5cda49535f https://hg.mozilla.org/mozilla-central/rev/c7e9e54de140
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•5 years ago
|
Version: Version 3 → 3 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•