Closed Bug 904743 Opened 7 years ago Closed 7 years ago

Create Python function for writing make dependency files

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: gps, Assigned: glandium)

References

Details

Attachments

(3 files, 7 obsolete files)

1.50 KB, patch
gps
: review+
Details | Diff | Splinter Review
8.78 KB, patch
gps
: review+
Details | Diff | Splinter Review
10.28 KB, patch
gps
: review+
Details | Diff | Splinter Review
We have a number of places in the tree where we write make dependency files in Python. This is easy enough to do by itself. However, we typically throw in non-trivial logic, such as wanting to add dependencies on loaded Python modules.

As glandium pointed out in bug 850380, we should probably consolidate this logic in a central location so we don't constantly reinvent it.
Is this too simple?

https://tbpl.mozilla.org/?tree=Try&rev=370e77b07097
Attachment #793161 - Flags: review?(mh+mozilla)
Assignee: nobody → gps
I was looking at making something more complete for bug 905973
Now with adding Python module dependencies.

https://tbpl.mozilla.org/?tree=Try&rev=f65231562176
Attachment #793173 - Flags: review?(mh+mozilla)
Attachment #793161 - Attachment is obsolete: true
Attachment #793161 - Flags: review?(mh+mozilla)
Hijacking
Assignee: gps → mh+mozilla
The normalize_args business is there to make both "manual" and "automatic" additions nice.

That is, you can both:
  rule.add_targets('foo', 'bar')
and
  rule.add_targets(func_returning_an_iterator())

Supporting a single argument iterator makes things awkward API wise imho for rule.add_targets(['foo']), and using a varadic args list makes things awkward with rule.add_targets(*func_returning_an_iterator()).
Attachment #793322 - Flags: review?(gps)
While at it, fix dependencies generated by BindingGen.py
Attachment #793325 - Flags: review?(gps)
(with a typo fix)
Attachment #793339 - Flags: review?(gps)
Attachment #793325 - Attachment is obsolete: true
Attachment #793325 - Flags: review?(gps)
Blocks: 905973
Note the add_* functions could return self, which would allow to do:
  makefile.create_rule('foo').add_dependencies('bar', 'baz').add_command('$(command) $@')

Not sure it's worth, though.
Attachment #793173 - Attachment is obsolete: true
Attachment #793173 - Flags: review?(mh+mozilla)
With a fix to the windows-only test.
Attachment #793443 - Flags: review?(gps)
Attachment #793322 - Attachment is obsolete: true
Attachment #793322 - Flags: review?(gps)
The makefiles the makeutil module outputs outsmart pymake's includedeps.
Attachment #793445 - Flags: review?(gps)
might have to remove normcase() because neither pymake nor make do the right thing :-/
Comment on attachment 793443 [details] [diff] [review]
Add helpers to create simple makefiles and iterate loaded python modules

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

Mostly looks good. I think you should just used named arguments instead of getting too magical with the argument detection.

::: python/mozbuild/mozbuild/makeutil.py
@@ +38,5 @@
> +            rule.dump(fh)
> +            for dep in rule.dependencies():
> +                all_deps.add(dep)
> +            for target in rule.targets():
> +                all_targets.add(target)

.update(rule.foo())

@@ +67,5 @@
> +                   command1
> +                   command2
> +                   ...
> +    '''
> +    def __init__(self, *targets):

What's wrong with just passing in an iterable? I guess the simple case of a single target is a little annoying - you have to wrap with [target] at the call site or else you get single character targets, which is probably wrong. Perhaps you should have multiple named arguments:

Rule(target='foo')
Rule(targets=get_targets())

@@ +69,5 @@
> +                   ...
> +    '''
> +    def __init__(self, *targets):
> +        targets = normalize_args(targets)
> +        self._targets = set(os.path.normcase(target).replace(os.sep, '/') for target in targets)

Shouldn't you call self.add_targets()?

@@ +89,5 @@
> +        self._commands.extend(normalize_args(commands))
> +
> +    def targets(self):
> +        '''Return an iterator on the rule targets.'''
> +        return iter(self._targets)

No need to return iter() - you can just return the instance itself.
Attachment #793443 - Flags: review?(gps) → feedback+
Comment on attachment 793445 [details] [diff] [review]
Allow multiple targets on the same rule with includedeps in pymake.

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

I wonder if this translates into a noticeable speedup. It surely can't hurt.
Attachment #793445 - Flags: review?(gps) → review+
Comment on attachment 793339 [details] [diff] [review]
Use makefile creation helper in BindingGen.py, cl.py and xpidl-process.py

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

Cancelling review until API is sorted out. From a cursory glance it looks good though. I do wonder if we should hook a FileAvoidWrite helper up to makeutils.py.
Attachment #793339 - Flags: review?(gps) → feedback+
(In reply to Gregory Szorc [:gps] from comment #12)
> What's wrong with just passing in an iterable? I guess the simple case of a
> single target is a little annoying - you have to wrap with [target] at the
> call site or else you get single character targets, which is probably wrong.
> Perhaps you should have multiple named arguments:
> 
> Rule(target='foo')
> Rule(targets=get_targets())

Too error-prone, and probably too verbose, imho.

> @@ +89,5 @@
> > +        self._commands.extend(normalize_args(commands))
> > +
> > +    def targets(self):
> > +        '''Return an iterator on the rule targets.'''
> > +        return iter(self._targets)
> 
> No need to return iter() - you can just return the instance itself.

The point was to give something out that's essentially read-only. Returning self._targets would make things like self.targets().add('foo') work.
(In reply to Gregory Szorc [:gps] from comment #14)
> Comment on attachment 793339 [details] [diff] [review]
> Use makefile creation helper in BindingGen.py, cl.py and xpidl-process.py
> 
> Review of attachment 793339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I do wonder if we should hook a FileAvoidWrite helper up to makeutils.py.

I wondered that too, but that felt like the wrong API level to do. You can just use a FileAvoidWrite with Makefile.dump().
Depends on: 908052
Attachment #793443 - Attachment is obsolete: true
Small fixup in create_rule.
Attachment #793858 - Flags: review?(gps)
Attachment #793856 - Attachment is obsolete: true
Attachment #793856 - Flags: review?(gps)
While at it, fix dependencies generated by BindingGen.py
Attachment #793859 - Flags: review?(gps)
Attachment #793339 - Attachment is obsolete: true
Attachment #793858 - Flags: review?(gps) → review+
Comment on attachment 793859 [details] [diff] [review]
Use makefile creation helper in BindingGen.py, cl.py and xpidl-process.py

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

::: dom/bindings/BindingGen.py
@@ +29,1 @@
>      with open(depsname, 'wb') as f:

Probably don't need binary mode here.

::: python/mozbuild/mozbuild/action/xpidl-process.py
@@ +12,5 @@
>  import sys
>  
>  from io import BytesIO
>  
> +from buildconfig import topsrcdir

I'm not crazy about importing buildconfig here because I'm not crazy about buildconfig. But, we can always clean it up later, so meh.
Attachment #793859 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/f2a4a8f5c9a5
https://hg.mozilla.org/mozilla-central/rev/5791f8bba150
https://hg.mozilla.org/mozilla-central/rev/bfc0c8ad9608
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 909415
Duplicate of this bug: 908993
Duplicate of this bug: 909035
Duplicate of this bug: 909737
Depends on: 912327
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.