Hi Brock,
On Thu, 2011-04-21 at 01:39 -0700, Brock Pytlik wrote:
> In general this looks good to me.
> Only comments I have are:
> - Could the test suite do a fast error out if the svr4 package needed to
> test isn't there? (Or alternatively do the make package itself?) Not a
> huge deal if those aren't possible.
Yep. The intent of the test-skipping was so that it'd report those
tests as being skipped, rather than causing the tests to error-out
altogether.
This also anticipates the future potential for long-running tests that
we may want to avoid under certain circumstances, or doing cut-down test
runs (the suite is getting rather long at this stage).
For example, having removed the svr4 packages directory, we get:
-----
timf@linn[3016] ./run.py -o t_importer
WARNING: You don't seem to be root. Some tests may fail.
# Accessibility not enabled, GUI tests disabled.
# logging to /tmp/tmpkkH2E4.pkg-test.log
..
# ----------------------------------------------------------------------
# Ran 2 tests in 5.212s - skipped 2 tests.
----
or
----
timf@linn[3018] ./run.py -v -o t_importer
WARNING: You don't seem to be root. Some tests may fail.
# Loading api tests:
.
.
distro-import.t_importer.py TestImporter.test_importer_basics match
pass
distro-import.t_importer.py TestImporter.test_importer_failures match
pass
# Ran 2 tests in 4.813s - skipped 2 tests.
Skipped tests:
distro-import.t_importer.py TestImporter.test_importer_basics: Test
Skipped: /home/timf/projects/ips/importer-tests-pkg.hg/packages/i386/svr4 does
not exist
distro-import.t_importer.py TestImporter.test_importer_failures: Test
Skipped: /home/timf/projects/ips/importer-tests-pkg.hg/packages/i386/svr4 does
not exist
----
> In importer.py, I'm not sure the conflicting variants code works if
> there's only one action in the list, and it's unvarianted. It looks to
> me like it would return that there are conflicts.
Fair point - that's guarded by the if-statement on line 411 that
continues if we only have one entry in the duplicate action list.
> I think you could also compress this code a little using
> itertools.combinations(action_list, 2) (thanks to Danek to pointing that
> function out to me a while back).
> I also know itertools can quickly move into the realm of magic, so
> totally up to you here.
Yep, I might leave that as is for now...
> Couldn't you have the same problem w/ hardlinks that you did with files?
> Ie, if a hardlink (or link) pointed at target a under one variant, but
> target b under another variant, wouldn't that cause problems around line
> 428?
Yes it will, good catch.
> Same question about the test on line 416, if the actions in dups were
> delivered under different variants, wouldn't that be ok? (ie, this path
> is a file on sparc, but a link on x86) Maybe those are situations where
> we tell people just don't do that.
Hmm - importer.py treats architectures separately and I don't think does
cross-checks between them, but other variants are a different story, and
yes, this would produce a false positive here.
> The lines 466-471 seem like the same thing could happen to be. We could
> deliver a directory with different modes under different variants (in
> theory, I think).
Yep.
> I'm fine if you want to say "not this bug" for all those issues and land
> the change, but those few similar circumstances caught me eye.
Those all sound like perfectly reasonable bugs (and I verified that,
with the right data files in the importer unit tests, we do indeed hit
the errors you predicted) I'd rather get this wad back sooner rather
than later. I've filed
18204 - importer.py should consider variants for links and different action
types
Thanks for taking a look,
cheers,
tim
>
> On 04/20/11 09:39 PM, Tim Foster wrote:
> > Hi there,
> >
> > I've got a fix here to stop the importer breaking when it encounters
> > actions that deliver to duplicate paths, but which are protected from
> > each other by correct variants.
> >
> > I've tagged this on to the import-tests wad that I originally sent out
> > back in October last year. I've added unit tests that check for this
> > breakage, in particular.
> >
> > While importer.py remains a crutch that we hope won't be long-lived,
> > every breakage is extremely disruptive and setting up a test environment
> > to reproduce errors turns out to be quite complex. Getting these back
> > to the gate would be good, but I'd rather not spend a lot more time
> > getting the unit tests perfected.
> >
> > http://cr.opensolaris.org/~timf/importer-tests-webrev
> >
> >
> > The other importer.py tests aren't as exhaustive, but I would like to
> > see any future importer.py fixes in the gate being accompanied by
> > tests to verify each change if at all possible please.
> >
> > Comments welcome?
> >
> > cheers,
> > tim
> >
> >
> >
> >
> >
> > _______________________________________________
> > pkg-discuss mailing list
> > [email protected]
> > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
>
> _______________________________________________
> pkg-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
