904743 - Create Python function for writing make dependency files

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Python API for writing make dependency files

Is this too simple?

https://tbpl.mozilla.org/?tree=Try&rev=370e77b07097

Comment 2

[Mike Hommey [:glandium]]

I was looking at making something more complete for bug 905973

Comment 3

[Gregory Szorc [:gps]]

Attachment: Python API for writing make dependency files

Now with adding Python module dependencies.

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

Comment 4

[Mike Hommey [:glandium]]

Hijacking

Comment 5

[Mike Hommey [:glandium]]

Attachment: Add helpers to create simple makefiles and iterate loaded python modules

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()).

Comment 6

[Mike Hommey [:glandium]]

Attachment: Use makefile creation helper in BindingGen.py, cl.py and xpidl-process.py

While at it, fix dependencies generated by BindingGen.py

Comment 7

[Mike Hommey [:glandium]]

Attachment: Use makefile creation helper in BindingGen.py, cl.py and xpidl-process.py

(with a typo fix)

Comment 8

[Mike Hommey [:glandium]]

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.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Add helpers to create simple makefiles and iterate loaded python modules

With a fix to the windows-only test.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Allow multiple targets on the same rule with includedeps in pymake.

The makefiles the makeutil module outputs outsmart pymake's includedeps.

Comment 11

[Mike Hommey [:glandium]]

might have to remove normcase() because neither pymake nor make do the right thing :-/

Comment 12

[Gregory Szorc [:gps]]

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.

Comment 13

[Gregory Szorc [:gps]]

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.

Comment 14

[Gregory Szorc [:gps]]

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.

Comment 15

[Mike Hommey [:glandium]]

(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.

Comment 16

[Mike Hommey [:glandium]]

(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().

Comment 17

[Mike Hommey [:glandium]]

Attachment: Add helpers to create simple makefiles and iterate loaded python modules

Comment 18

[Mike Hommey [:glandium]]

Attachment: Add helpers to create simple makefiles and iterate loaded python modules

Small fixup in create_rule.

Comment 19

[Mike Hommey [:glandium]]

Attachment: Use makefile creation helper in BindingGen.py, cl.py and xpidl-process.py

While at it, fix dependencies generated by BindingGen.py

Comment 20

[Gregory Szorc [:gps]]

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.

Comment 21

[Mike Hommey [:glandium]]

http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/2aaad4a880f9

Comment 22

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a4a8f5c9a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5791f8bba150
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc0c8ad9608

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2a4a8f5c9a5
https://hg.mozilla.org/mozilla-central/rev/5791f8bba150
https://hg.mozilla.org/mozilla-central/rev/bfc0c8ad9608

Comment 24

[Mike Hommey [:glandium]]

Fixup for bug 908977 comment 20.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f9e762ff152

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f9e762ff152