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)

3 Branch
enhancement
Not set
normal

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 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 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 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)
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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/ec5cda49535f
https://hg.mozilla.org/mozilla-central/rev/c7e9e54de140
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.