1043268 - Skip (sub)configures when it makes sense

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

One of the remaining causes of clobbers due to configure (if not the last) is when landing new nspr, icu or ffi. We should detect when we need to rerun subconfigures and rerun them even if the main configure doesn't need running.

On the opposite of the spectrum, when running the main configure because it changed, don't rerun subconfigures that aren't involved.

Comment 1

[Mike Hommey [:glandium]]

Forcing and skipping is different enough that I'd rather address them separately. Cloned as bug 1046553 for the forcing part.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Skip subconfigures when it makes sense

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8465234 [details] [diff] [review]
Skip subconfigures when it makes sense

This busts static analysis

Comment 4

[Mike Hommey [:glandium]]

Attachment: Skip subconfigures when it makes sense

https://tbpl.mozilla.org/?tree=Try&rev=4b32acd3ed9e

Comment 5

[Michael Shal [:mshal]]

Comment on attachment 8465278 [details] [diff] [review]
Skip subconfigures when it makes sense

>+    @property
>+    def modified(self):
>+        '''Returns whether the file was modified since the instance was
>+        created. Result is memoized.'''
>+        if hasattr(self, '_modified'):
>+            return self._modified
>+
>+        modified = True
>+        if os.path.exists(self._path):
>+            if open(self._path, 'rb').read() == self._content:
>+                modified = False
>+        self._modified = modified
>+        return modified

Is there a reason you needed this memoized? It looks like it only gets called once per file anyway.

>+        if config_files or command_files:

Why do we need this if statement?

>+            for f, t in config_files:
>+                if os.path.exists(f) and os.path.exists(t) and \
>+                        os.path.getmtime(f) < os.path.getmtime(t):
>+                    skip_config_status = False

Shouldn't the template (t) always exist? And if the output file (f) doesn't exist for some reason, shouldn't we set skip_config_status = False in that case as well?

Comment 6

[Mike Hommey [:glandium]]

(In reply to Michael Shal [:mshal] from comment #5)
> Comment on attachment 8465278 [details] [diff] [review]
> Skip subconfigures when it makes sense
> 
> >+    @property
> >+    def modified(self):
> >+        '''Returns whether the file was modified since the instance was
> >+        created. Result is memoized.'''
> >+        if hasattr(self, '_modified'):
> >+            return self._modified
> >+
> >+        modified = True
> >+        if os.path.exists(self._path):
> >+            if open(self._path, 'rb').read() == self._content:
> >+                modified = False
> >+        self._modified = modified
> >+        return modified
> 
> Is there a reason you needed this memoized? It looks like it only gets
> called once per file anyway.

There's actually another use of .modified, so this can happen twice. And I'd rather not re-read the file then.

> >+        if config_files or command_files:
> 
> Why do we need this if statement?

Because of the comment above it.

> >+            for f, t in config_files:
> >+                if os.path.exists(f) and os.path.exists(t) and \
> >+                        os.path.getmtime(f) < os.path.getmtime(t):
> >+                    skip_config_status = False
> 
> Shouldn't the template (t) always exist? And if the output file (f) doesn't
> exist for some reason, shouldn't we set skip_config_status = False in that
> case as well?

So, if not os.path.exists(f) or not os.path.exists(t) or os.path.getmtime(f) < os.path.getmtime(t)?

Comment 7

[Michael Shal [:mshal]]

> > >+        if config_files or command_files:
> > 
> > Why do we need this if statement?
> 
> Because of the comment above it.

Yeah, I read that - it still doesn't make sense to me. I must not be seeing it :). If there are no config_files but there are command_files, the if statement will be true, but doesn't the loop do nothing in that case? Similarly, it seems the 'if config_files' part of the statement is redundant with the loop itself. If the list is empty, there's nothing to loop over - no need to have the if beforehand to check.

> 
> > >+            for f, t in config_files:
> > >+                if os.path.exists(f) and os.path.exists(t) and \
> > >+                        os.path.getmtime(f) < os.path.getmtime(t):
> > >+                    skip_config_status = False
> > 
> > Shouldn't the template (t) always exist? And if the output file (f) doesn't
> > exist for some reason, shouldn't we set skip_config_status = False in that
> > case as well?
> 
> So, if not os.path.exists(f) or not os.path.exists(t) or os.path.getmtime(f)
> < os.path.getmtime(t)?

I guess I don't understand how the template can not exist. Isn't that part of the source tree?

Comment 8

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3878325b7e

Comment 9

[Mike Hommey [:glandium]]

Fixup for https://tbpl.mozilla.org/php/getParsedLog.php?id=45149991&tree=Mozilla-Inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ca6da63f025

Will file a separate bug for the real issue, the fixup is just a workaround.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d3878325b7e
https://hg.mozilla.org/mozilla-central/rev/5ca6da63f025