Closed Bug 1043268 Opened 6 years ago Closed 6 years ago

Skip (sub)configures when it makes sense

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: glandium, Assigned: glandium)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1043140
Assignee: nobody → mh+mozilla
Depends on: 1046045
Depends on: 1046533
Blocks: 1046553
Forcing and skipping is different enough that I'd rather address them separately. Cloned as bug 1046553 for the forcing part.
Summary: Force or skip (sub)configures when it makes sense → Skip (sub)configures when it makes sense
Attachment #8465234 - Flags: review?(mshal)
Comment on attachment 8465234 [details] [diff] [review]
Skip subconfigures when it makes sense

This busts static analysis
Attachment #8465234 - Flags: review?(mshal)
Attachment #8465234 - Attachment is obsolete: true
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?
Attachment #8465278 - Flags: review?(mshal) → review+
(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)?
> > >+        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?
Depends on: 1048072
https://hg.mozilla.org/mozilla-central/rev/5d3878325b7e
https://hg.mozilla.org/mozilla-central/rev/5ca6da63f025
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.