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

RESOLVED FIXED in mozilla30

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Blocks 1 bug)

unspecified
mozilla30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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.)
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 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/?
Attachment #8374526 - Flags: review?(gps)
Attachment #8374526 - Flags: feedback?(mh+mozilla)
(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 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.
Attachment #8374526 - Flags: review?(gps) → review+
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.
Attachment #8374527 - Flags: review?(gps) → review+
Attachment #8374528 - Flags: review?(gps) → review+
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.
Attachment #8374529 - Flags: review?(gps) → review+
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?
Attachment #8374526 - Flags: feedback?(mh+mozilla) → feedback+
Blocks: 972432
(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.
(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.
(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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.