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

RESOLVED FIXED in mozilla26

Status

RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

24 Branch
mozilla26
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
+++ 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.
(Assignee)

Comment 1

5 years ago
Created attachment 788079 [details] [diff] [review]
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.
Attachment #788079 - Flags: review?(gps)
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Comment 3

5 years ago
Created attachment 791647 [details] [diff] [review]
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).
Attachment #791647 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #788079 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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)
(Assignee)

Comment 5

5 years ago
Created attachment 791780 [details] [diff] [review]
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.)
Attachment #791780 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #791647 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 906403

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 792636 [details] [diff] [review]
Wrap subconfigure invocation and restore config.status produced file timestamps if they haven't changed
Attachment #792636 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Updated

5 years ago
Depends on: 962758
(Assignee)

Updated

4 years ago
Blocks: 1046533

Updated

6 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.