Tup build invocation machinery should know when to run configure

RESOLVED FIXED in Firefox 62

Status

enhancement
RESOLVED FIXED
Last year
6 months ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Blocks 1 bug)

3 Branch
mozilla62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

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 3

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/ec5cda49535f
https://hg.mozilla.org/mozilla-central/rev/c7e9e54de140
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1463921
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.