On 07/20/12 15:04, Danek Duvall wrote:
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.
[snip]
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.
Yeah, I think I wrote that comment when I was still planning on
reworking the lookup code. Even if it's fixed though, I'd leave the
comment (or restore it to say efficiently) since if we know which
publisher we're looking for, that will always be at least as fast as not
knowing.
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.
Ok, I'll file a bug on that.
As a side note, I think I was working on the assumption that it was
easier and simpler to just grab all ro data (which in practice was 90+%
signing data) for tests which needed it (which in practice was 90+% of
tests that needed ro data) since having extra data didn't really cost
much and kept the number of flags to a minimum. If that balance has
shifted, I'm happy for someone to change it as desired.
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.
Sure.
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.
I agree that something would be awfully wrong. If I'd made such a change
in my gate though, having it fail before I change the property instead
of after would probably save my a lot of headache while trying to figure
out what went wrong. I can remove it if you prefer.
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.
Ah. Ok. I agree it should return 1.
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.
I thought we did but perhaps we don't. I'll rework this code to deal
with potentially scrambled manifests.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
