903369 - Running configure with no configure.in change overwrites some files under libffi

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

+++ This bug was initially created as a clone of Bug #903321 +++

Running configure re-runs subconfigures, which, for non-ConfigStatus.py ones, means config.status generated files are overwritten even if not changed. This makes ffi.h have a new modification time, which, in turn, makes a few CTypes files rebuild, as well as libjs_static.a and all the files that link against it, including libxul.so.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed

This brings me from 124 files after bug 903321 and bug 903341, down to 4 after touch configure.in.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 788079 [details] [diff] [review]
Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed

sigh. Thanks to msys path munging, this breaks on windows.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed

I think I got this right this time. At least it works locally on windows. Note this also addresses the issue with jemalloc3 (because it uses a subconfigure) and icu (manually ; runConfigureICU is just a wrapper around ICU's configure, so the strategy works there too).

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 791647 [details] [diff] [review]
Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed

Shoot. It passes configure, now, but fails during build.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed

This finally works on windows, by not having the subconfigure itself called by python, which makes things almost impossible to deal with (msys blows environment and arguments, and it's very hard to make it right without blowing something else up.)

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 791780 [details] [diff] [review]
Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed

Review of attachment 791780 [details] [diff] [review]:
-----------------------------------------------------------------

This will get an r+ once feedback is addressed. That being said, parsing the output of config.status feels overly hacky to me. Not r+ blocking hacky. Just hacky.

Surely we could overload some m4 macro to capture the set of generated files. Although, once we do that, I suppose we should just have file generation go through a Python script that performs the FileAvoidWrite magic. How much extra work is that? Followup bug territory?

::: build/subconfigure.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import os

Please add a short comment at the top of this file documenting it's purpose.

@@ +11,5 @@
> +
> +class File(object):
> +    def __init__(self, path):
> +        self._path = path
> +        self._content = open(path, 'r').read()

Since we're doing a content compare, I'd feel much better if the comparison were done in binary mode.

@@ +26,5 @@
> +
> +def dump(shell):
> +    if not os.path.exists('config.status'):
> +        if os.path.exists(CONFIG_DUMP):
> +            os.remove(CONFIG_DUMP)

I'd feel better if the path were passed as an argument.

@@ +32,5 @@
> +
> +    config_files = [File('config.status')]
> +
> +    # Scan the config.status output for information about configuration files
> +    # it generates.

Parsing the output of config.status - what could possibly go wrong?

@@ +33,5 @@
> +    config_files = [File('config.status')]
> +
> +    # Scan the config.status output for information about configuration files
> +    # it generates.
> +    config_status_output = subprocess.check_output([shell, '-c', './config.status --help'], stderr=subprocess.STDOUT).splitlines()

Nit: Wrap.

@@ +45,5 @@
> +            for f in (couple.split(':')[0] for couple in line.split()):
> +                if os.path.isfile(f):
> +                    config_files.append(File(f))
> +
> +    with open(CONFIG_DUMP, 'w') as f:

Don't you need binary mode for pickle?

@@ +50,5 @@
> +        pickle.dump(config_files, f)
> +
> +
> +def adjust():
> +    if not os.path.exists(CONFIG_DUMP):

Path as argument, please.

@@ +59,5 @@
> +    try:
> +        with open(CONFIG_DUMP, 'r') as f:
> +            config_files = pickle.load(f)
> +    finally:
> +        pass

finally: pass? Did you mean to write "except"?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #6)
> Surely we could overload some m4 macro to capture the set of generated
> files. Although, once we do that, I suppose we should just have file
> generation go through a Python script that performs the FileAvoidWrite
> magic. How much extra work is that? Followup bug territory?

Those are for third party directories. With checked-in configure. That use different versions of autoconf. I'd rather not have to run autoconf on those.

Comment 8

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Gregory Szorc [:gps] from comment #6)
> > Surely we could overload some m4 macro to capture the set of generated
> > files. Although, once we do that, I suppose we should just have file
> > generation go through a Python script that performs the FileAvoidWrite
> > magic. How much extra work is that? Followup bug territory?
> 
> Those are for third party directories. With checked-in configure. That use
> different versions of autoconf. I'd rather not have to run autoconf on those.

Then hacky solution I fear it is :/ Would be nice to have some kind of sanity check, if possible.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 792636 [details] [diff] [review]
Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed

Review of attachment 792636 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/subconfigure.py
@@ +61,5 @@
> +
> +    try:
> +        with open(dump_file, 'rb') as f:
> +            config_files = pickle.load(f)
> +    except:

except Exception:

(otherwise this captures SystemExit and KeyboardInterrupt and you have a bad day)

Comment 11

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f98e1ea1ef

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6f98e1ea1ef