893976 - Integrate update-uuids with mach

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Yes, perl, ugh; it works, though.

Comment 1

[Gregory Szorc [:gps]]

You didn't tell me it was new perl! It's been a few years, but I do know perl, so I should be able to review this. I'm treating this as somewhat low in the priority stack. Expect turnaround in a few days.

Comment 2

[Steve Fink [:sfink] [:s:]]

(In reply to Gregory Szorc [:gps] from comment #1)
> You didn't tell me it was new perl! It's been a few years, but I do know
> perl, so I should be able to review this. I'm treating this as somewhat low
> in the priority stack. Expect turnaround in a few days.

It's an import of http://people.mozilla.org/~sfink/data/update-uuids

So, it's not new, but it's new to the tree, and has never been reviewed. Poor gps. I didn't exactly write it with review in mind.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yeah, it wasn't new to me :) No hurry in getting it reviewed.

Comment 4

[Mike Hommey [:glandium] OoO until Dec 30]

The trend is to *remove* perl code, not add some.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yes. This is a pre-existing, useful tool, though. I'd be very happy to see someone rewrite it in python, but I don't know enough perl to do that myself.

Comment 6

[(not currently active) Ted Mielczarek]

I'm not wild about adding new Perl code anywhere, but adding it for a mach utility is less offensive than adding it to a functional part of the build system, which I would strongly oppose.

Comment 7

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 775849 [details] [diff] [review]
Patch v1

I have a working prototype in python, which happens to work better in edge cases (the perl implementation is confused with different files containing the same interface, and when doing update-uuids . nsIDOMNode, it skips dom/interfaces/xul/nsIDOMXULDocument.idl, dom/interfaces/xul/nsIDOMXULElement.idl and dom/interfaces/xul/nsIDOMXULTreeElement.idl).

Comment 8

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Allow to skip FileFinder executables detection

This is not immediately obvious, but as my prototype uses FileFinder, this makes FileFinder faster for it.

Comment 9

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Use (cached) regular expressions for mozpack.path.match

This too makes FileFinder significantly faster.

Comment 10

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Prototype

This is the prototype implementation, leveraging the python xpidl parser and useful tools from the packager. The non-class part is to integrate within mach. I just didn't know where to put everything.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 778318 [details] [diff] [review]
Use (cached) regular expressions for mozpack.path.match

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

::: python/mozbuild/mozpack/path.py
@@ +96,5 @@
>      '''
>      if not pattern:
>          return True
> +    if not pattern in re_cache:
> +      pattern = re.escape(pattern)

Nit: 4-space

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 778314 [details] [diff] [review]
Allow to skip FileFinder executables detection

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

So I guess this makes it faster because we avoid a file stat completely and just go off info from listdir? Perhaps a comment should be added to FileFinder.__init__ documenting this.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 778318 [details] [diff] [review]
Use (cached) regular expressions for mozpack.path.match

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

::: python/mozbuild/mozpack/packager/formats.py
@@ +264,4 @@
>          if any(mozpack.path.match(path, p.replace('*', '**'))
>                 for p in self._non_resources):
>              return False
> +        path = mozpack.path.split(path)

This chunk seems unrelated to the rest of the patch. Did this accidentally creep in?

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 778318 [details] [diff] [review]
Use (cached) regular expressions for mozpack.path.match

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

Oh, r+ conditional on 4-space indent :)

Comment 15

[Gregory Szorc [:gps]]

I'd put things somewhere under /tools.

Comment 16

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 778318 [details] [diff] [review]
Use (cached) regular expressions for mozpack.path.match

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

::: python/mozbuild/mozpack/packager/formats.py
@@ +264,4 @@
>          if any(mozpack.path.match(path, p.replace('*', '**'))
>                 for p in self._non_resources):
>              return False
> +        path = mozpack.path.split(path)

path = mozpack.path.split(mozpack.path.relpath(path, base))
became two separate steps, in between which match is now done. Because match doesn't accept the split patch any more, but the rest of the code requires split path.

Comment 17

[Mike Hommey [:glandium] OoO until Dec 30]

(In reply to Gregory Szorc [:gps] from comment #15)
> I'd put things somewhere under /tools.

For a mach command?

Comment 18

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #17)
> (In reply to Gregory Szorc [:gps] from comment #15)
> > I'd put things somewhere under /tools.
> 
> For a mach command?

I don't know where else we'd put it! We already have /tools/mach_commands.py if you don't want to create a new directory.

Comment 19

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Don't create xpidl_debug and avoid LALR tables generation message

I think at this point it makes no sense to keep generating the xpidl_debug parser output, and bothering people with the "Generating LALR tables" message.

Comment 20

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Avoid importing buildconfig globally from mozpack.executables

The reason for this is that when running the update-uuid code using the FileFinder, we end up importing buildconfig, which wants to be running from the virtualend and find a config.status. But we also don't actually need buildconfig for anything, so just avoid importing it globally.

Comment 21

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Add a mach command to update uuids for specific interfaces and their descendants

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 781578 [details] [diff] [review]
Add a mach command to update uuids for specific interfaces and their descendants

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

Thanks!

::: tools/mach_commands.py
@@ +66,5 @@
> +    from, what its uuid is, and where in the source file the uuid is.
> +    '''
> +    def __init__(self, filename, production):
> +        import xpidl
> +        assert(isinstance(production, xpidl.Interface))

I think 'assert foo' is more idiomatic than 'assert(foo)'.

@@ +157,5 @@
> +    @CommandArgument('path', nargs='?',
> +                     help='Base path under which uuids will be searched (optional).')
> +    @CommandArgument('interfaces', nargs='+',
> +                     help='Changed interfaces whose UUIDs need to be updated. ' +
> +                          'Their descendants are updates as well.')

updated

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 781578 [details] [diff] [review]
Add a mach command to update uuids for specific interfaces and their descendants

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

I kinda wish the code lived outside mach_commands.py, but meh.

::: tools/mach_commands.py
@@ +166,5 @@
> +        import mozpack.path
> +        from tempfile import mkdtemp
> +
> +        # If more than one interface is given, but no path, mach will give
> +        # the first interface as 'path'.

I'd just avoid this whole mess by using --path.

@@ +179,5 @@
> +        tmpdir = mkdtemp()
> +        try:
> +            parser = xpidl.IDLParser(outputdir=tmpdir)
> +            registry = InterfaceRegistry()
> +            for p, f in finder.find('**/*.idl'):

This will find objdir/dist/idl/*.idl, right? Do we care?

@@ +184,5 @@
> +                p = mozpack.path.join(path, p)
> +                try:
> +                    content = f.open().read()
> +                    idl = parser.parse(content, filename=p)
> +                except:

except Exception (since we don't want to trap KeyboardInterrupt).

Comment 24

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Add a mach command to update uuids for specific interfaces and their descendants

Re-running it by you for the CommandArgument stuff.

Comment 25

[Mike Hommey [:glandium] OoO until Dec 30]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ff3875a6f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eeea818324c
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb60c9b4b74f
https://hg.mozilla.org/integration/mozilla-inbound/rev/03f984a3bd2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9251bba8a9b2

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e3ff3875a6f0
https://hg.mozilla.org/mozilla-central/rev/5eeea818324c
https://hg.mozilla.org/mozilla-central/rev/fb60c9b4b74f
https://hg.mozilla.org/mozilla-central/rev/03f984a3bd2a
https://hg.mozilla.org/mozilla-central/rev/9251bba8a9b2