Brock Pytlik wrote: > On 07/19/12 19:46, Danek Duvall wrote: > >Brock Pytlik wrote: > > > >>https://cr.opensolaris.org/action/browse/pkg/bpytlik/7139940-v1 > >image.py: > > > - line 2368: Is the is_pkg_installed() test sufficiently faster than the > > manifest verification test that we really need it? I would think that > > if we had a corrupt manifest, we might want to do the verification, > > regardless of whether the package was installed. > > The is_pkg_installed check isn't there for speed. It's there because a > least one team member said that we couldn't change the manifest on disk > if the package was installed since that would be modifying the image > state even if we were only doing (for example) a pkg contents.
Sure, okay. I agree with that; I guess in trying to figure out exactly what that code did, it looked like it was a duplicate check, but I guess I wasn't thinking about the case that a manifest would be on disk, but not installed. > >transport.py: > > > > - I would put this change into image.py instead -- have a private > > __verify_manifest() that called _verify_manifest(), caught the > > exception, and returned true or false. > I can do that, but I think I have two issues with it. The first is that then > we're calling a protected method from another class. We do that frequently; the single underscore is a mechanism to denote that the class member isn't intended to be used by the general public, but by other parts of our own code (and not just restricted to subclasses). > The second is that I don't think _verify_manifest is an appropriate > "public" interface because it raises the exception and having the method > verify_manifest which returns true or false seemed more appropriate to > me. It still wouldn't be a public interface, just one that image.py uses directly. My reasoning is that users of the low-level transport interfaces are probably going to be sophisticated enough to catch the exception themselves, and that the one place where you're looking specifically for the true/false is in the image code, so the image code should just contain that helper function itself. > If what I've said here hasn't convinced you, let's talk offline and > see if we can reach a conclusion. Sure. > >dependencies.py: > > > > - I'm assuming this change is purely for performance, and not really > > related to either bug? > Well, the removal of the if system_patterns is just removal of completely > silly code that got in there accidentally. > The other code change is related to the changes made elsewhere. > Specifically, is_pkg_installed doesn't work with fmris that don't have > publishers. I looked into fixing that (since that seems kinda broken to me) > but it involved an unfortunately large amount of change. Since the only > place we called get_manifest with a fmri without a publisher was from the > dependency code, that seemed like the expedient change for now, with an RFE > to be filed for is_pkg_installed. Maybe I was misled by "efficiently"? If it doesn't work at all, then perhaps you should say so. You might file a bug against this (if there isn't one already), along with the note that this code should be tweaked again once the fix is in. > >pkg5unittest.py > > > > - line 2222, 2264: these could be commonized, no? And why test against > > need_ro_data? Do all the need_ro_data tests do signing, too? Or is it > > just low-enough overhead that it's okay to do to all tests where > > need_ro_data is True? > Sure, they can be commonized, I just didn't really see a reason to. > For now, need_ro_data is the only switch we have. Initially, the only tests > that needed ro_data were signing tests. That may have changed slightly since > it was initially created, but I think it's still true for the vast majority > of tests (I'd guess all but 1 or 2). Given that, I think what we have works > fine for now. If someone wants to create more fine grained switches, I'm not > opposed, but I don't think it's needed at this time. I can take the commonization or leave it, but having "need_ro_data" really mean "I sign packages" is something I think needs to be changed. It's a matter of maintainability of the testsuite as a whole. > >I don't understand the "PerTestRepo" class naming scheme. Why do some > >files get new classes and others don't? > If I remember right, the files that got new classes had persistent setup for > their other tests. Since I'm these tests explicitly muck around with > packages in the repo, I wanted those in classes which didn't have persistent > setup. That's what I was trying to convey with "PerTestRepo" ie, each test > gets its own repo. Other naming suggestions welcomed. Oy. :( Please add a comment to that effect for each of those classes. I think this is something that can't simply be conveyed in a name. > >t_fix.py: > > > > - line 330: "runing" -> "running" > > > > - line 336: why this first fix? Isn't this always a no-op? > To make sure that the reason the fix fails is because the package isn't > signed, and not any other reason. I'm surprised that you're worrying about a package that's just been installed being in a non-verifiable state. If that's happening, something's awfully wrong. > >t_pkg_contents.py: > > > > - line 309, 310: editorial comment: pkg contents -t<type> should > > probably return 1 if there is no action of type<type> in the package. > I'll check to make sure it does. Assuming it does, then I could change this > to a single command which expected an exit status of 1, correct? Sorry, I wasn't clear. It doesn't, but I think it should, and wanted folks opinions before I went and filed a bug. > >t_pkg_info.py: > > > > - line 649: is this really guaranteed? > It is given the particular manifest we're working on. Why? Do we guarantee action ordering in manifests when we publish them? We'd assert if the license ended up in the manifest before the set action. I think that simply using the Manifest class to iterate over actions by type would be safer. Or we can do that if and when we hit this assert. Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
