971272 - Guard against adding {foo, foo/bar} to FileRegistry

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

This already raised if the order was [foo, foo/bar].  But it didn't prevent adding [foo/bar, foo].

The only sub-classes of FileRegistry are FileCopier and Jarrer.

FileCopier.copy threw in the previously unhandled case: the order of creation is the same as the order of addition, so that foo is created after foo/bar, which alwayas fails.

A zip file index can contain both foo and foo/bar.  I don't think we should rely on this property in our use of Jarrer, but if we already do, I guess we need to move these guards into FileCopier.  Let's hope that's not the case!

(For the record: On my Mac OS X system, unzipping such a zip file prompts the user for what to do, depending on the order of the entries in the zip index.)

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Guard against adding {foo, foo/bar} to FileRegistry. r=gps

This already raised if the order was [foo, foo/bar].  But it didn't
prevent adding [foo/bar, foo].

The only sub-classes of FileRegistry are FileCopier and Jarrer.

FileCopier.copy threw in the previously unhandled case: the order of
creation is the same as the order of addition, so that foo is created
after foo/bar.

A zip file index can contain both foo and foo/bar.  I don't think we
should rely on this property in our use of Jarrer, but if we already do,
I guess we need to move these guards into FileCopier.  Let's hope that's
not the case!

(For the record: On my Mac OS X system, unzipping such a zip file
prompts the user for what to do, depending on the order of the entries
in the zip index.)

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Expose FileRegistry.required_directories. r=gps

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Use required_directories in FileCopier.copy. r=gps

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Post: Add informational test showing unnecessary directories may be created. r=gps

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8374526 [details] [diff] [review]
Part 1: Guard against adding {foo, foo/bar} to FileRegistry. r=gps

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

glandium: can you speak to whether Jarrer requires a file foo and a path foo/?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #4)
> Created attachment 8374529 [details] [diff] [review]
> Post: Add informational test showing unnecessary directories may be created.
> r=gps

For the record, this behaviour is the same before and after my patches.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comprehensive try build at:

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

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8374526 [details] [diff] [review]
Part 1: Guard against adding {foo, foo/bar} to FileRegistry. r=gps

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

LGTM.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8374527 [details] [diff] [review]
Part 2: Expose FileRegistry.required_directories. r=gps

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

::: python/mozbuild/mozpack/test/test_copier.py
@@ +122,5 @@
> +        self.registry.add('foo', GeneratedFile('foo'))
> +        self.assertEqual(self.registry.required_directories(), set())
> +
> +        self.registry.add('bar/baz', GeneratedFile('barbaz'))
> +        self.assertEqual(self.registry.required_directories(), set(['bar']))

Lesser known fact: {'bar'} is syntactical sugar for creating a set. Essentially {} without : is the set literal.

@@ +140,5 @@
> +        self.registry.remove('bar/zot')
> +        self.assertEqual(self.registry.required_directories(), set([]))
> +
> +        self.registry.add('x/y/z', GeneratedFile('xyz'))
> +        self.assertEqual(self.registry.required_directories(), set(['x', 'x/y']))

I probably would have rolled these tests inline with other tests. But this is fine.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8374529 [details] [diff] [review]
Post: Add informational test showing unnecessary directories may be created. r=gps

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

*sigh*.

Add an in-line comment saying the behavior is wrong and the test merely exists to guard against unexpected changes in behavior and check it in. Bonus points if you file follow-ups on the issues and drop the bug number in the code.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8374526 [details] [diff] [review]
Part 1: Guard against adding {foo, foo/bar} to FileRegistry. r=gps

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

::: python/mozbuild/mozpack/copier.py
@@ +43,5 @@
> +        while partial_path:
> +            partial_path = mozpack.path.dirname(partial_path)
> +            if partial_path:
> +                partial_paths.append(partial_path)
> +        return partial_paths

Why not just add to _required_directories for each iteration of the original loop instead?

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #11)
> Comment on attachment 8374526 [details] [diff] [review]
> Part 1: Guard against adding {foo, foo/bar} to FileRegistry. r=gps
> 
> Review of attachment 8374526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozpack/copier.py
> @@ +43,5 @@
> > +        while partial_path:
> > +            partial_path = mozpack.path.dirname(partial_path)
> > +            if partial_path:
> > +                partial_paths.append(partial_path)
> > +        return partial_paths
> 
> Why not just add to _required_directories for each iteration of the original
> loop instead?

I could, but I need _partial_paths for .remove as well, so there's a nice update/subtract symmetry.

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 8374529 [details] [diff] [review]
> Post: Add informational test showing unnecessary directories may be created.
> r=gps
> 
> Review of attachment 8374529 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> *sigh*.
> 
> Add an in-line comment saying the behavior is wrong and the test merely
> exists to guard against unexpected changes in behavior and check it in.
> Bonus points if you file follow-ups on the issues and drop the bug number in
> the code.

Filed Bug 972432 and updated the comment.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8374527 [details] [diff] [review]
> Part 2: Expose FileRegistry.required_directories. r=gps
> 
> Review of attachment 8374527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozpack/test/test_copier.py
> @@ +122,5 @@
> > +        self.registry.add('foo', GeneratedFile('foo'))
> > +        self.assertEqual(self.registry.required_directories(), set())
> > +
> > +        self.registry.add('bar/baz', GeneratedFile('barbaz'))
> > +        self.assertEqual(self.registry.required_directories(), set(['bar']))
> 
> Lesser known fact: {'bar'} is syntactical sugar for creating a set.
> Essentially {} without : is the set literal.

This was not the case not so long ago, so thanks for the update.  Even lesser known consequence: since {} doesn't create a set, it fails assertEqual.

Comment 15

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a808e19f51c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1870df76f99
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6754df3f99
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f2b18e87f1

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a808e19f51c1
https://hg.mozilla.org/mozilla-central/rev/a1870df76f99
https://hg.mozilla.org/mozilla-central/rev/fe6754df3f99
https://hg.mozilla.org/mozilla-central/rev/31f2b18e87f1