Closed Bug 903369 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, defect)

24 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ 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.
This brings me from 124 files after bug 903321 and bug 903341, down to 4 after touch configure.in.
Attachment #788079 - Flags: review?(gps)
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.
Attachment #788079 - Flags: review?(gps)
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).
Attachment #791647 - Flags: review?(gps)
Attachment #788079 - Attachment is obsolete: true
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.
Attachment #791647 - Flags: review?(gps)
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.)
Attachment #791780 - Flags: review?(gps)
Attachment #791647 - Attachment is obsolete: true
Blocks: 906403
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"?
Attachment #791780 - Flags: review?(gps) → feedback+
(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.
(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.
Attachment #791780 - Attachment is obsolete: true
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)
Attachment #792636 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/f6f98e1ea1ef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 962758
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.