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 2362: now that has_manifest() is doing something more interesting,
we should have a doc string for it.
Ok.
- 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. This
seemed reasonable to me and what I think we need to do, under a separate
RFE, is have pkg fix or some other command, handle the fixing of a
corrupted manifest. It's not as simple as one might hope since we'd need
to have both the corrupt and correct manifest around at the same time,
which involved more code change than I was looking to make in this
situation.
- line 2438: couldn't you just call has_manifest()?
Looks like it. I'll make that change.
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. 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. If what I've
said here hasn't convinced you, let's talk offline and see if we can
reach a conclusion.
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.
- line 1647: "a" -> "an"
- line 1653: "fmri's" -> "fmris"
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 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.
t_change_variant.py:
- line 484: the parens around self.pkg_shared don't mean anything here; a
1-tuple needs a trailing comma, but you can pass just the single
string. (Ditto in some of the other files.)
Yeah... not sure what I was thinking there.
- I'm not sure how this test is supposed to work, though; would the
second call to change-variant succeed with the current code?
The same thing would happen with the current code, which is what's
desired. What we're testing here is that we don't overwrite an installed
package's manifest even if it's no longer what's in the repo.
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.
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?
t_pkg_info.py:
- line 649: is this really guaranteed?
It is given the particular manifest we're working on.
- You should be able to put the license mangling into a common function.
Sure.
t_pkg_install.py:
- line 1013: I'm not sure that this ordering is guaranteed.
It certainly seems to be, but if you prefer, I'll run through all the
lines and look for the one that names the package.
- line 1016: I wouldn't guarantee that the rebuild would succeed with
this modification. Perhaps you could just rewrite a valid manifest on
top of m_path that simply omits the /usr/bin/cat line?
Sure.
Thanks for the look,
Brock
Thanks,
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
