On 06/16/11 11:52, Danek Duvall wrote:
Brock Pytlik wrote:
http://cr.opensolaris.org/~bpytlik/ips-18441-v1/
pkg.1.txt:
- You should mention some of the well-defined error cases, such as
freezing an installed package at a version that isn't installed. Also,
what happens when you freeze a package that's already frozen? As far
as I could tell from the code, the freeze is simply updated with the
new value (assuming that passed all the matching checks).
ok
client.py:
- line 2172: the message should be put in _()
ok
- line 2181: I see what you're doing here, but I wonder if people will
get confused by seeing a version they didn't specify.
I thought that was useful for the 'pkg freeze foo' case, but I don't
really have strong feelings. If you have an idea for what the output
should look like, please let me know.
api_errors.py:
- line 2718, et al: why include the initial newline?
I forgot to put a slash, I'll add them.
- line 2721, et al: I'd eliminate the last newline by just putting the
triple quotes at the end of this line.
Sure.
image.py:
- I'm thinking we might want a timestamp associated with each freeze,
too.
Ok. When would you like that shown/reported? Should it be part of the
message that's shown when a update fails because of a freeze? Is there a
particular date/time format you'd like when it's displayed?
- line 3681, et al: no need to specify "build_release", just "5.11" as
the parameter will do fine; it's what we do everywhere else.
ok
- line 4061, et al: "pacakges" -> "packages"
ok
- line 4066: we need better handling than an assert here. We should also
handle other forms of corruption here. What happens if the dict
doesn't have the form we expect, or the file is truncated, etc. Any
interesting json failure modes?
Since we only have one version at the moment, an assert seemed fine to
me, but sure, I can change it to raise an exception. Guess I'll go see
what other failure modes we account for w/ json in other places, and
file a bug for avoid.
- line 4076: why not inside the try?
thinko
imageplan.py:
- line 2308: not your change, but "peformed" -> "performed".
sure
- match_user_stems(): The changes here feel awkward, though I'd
appreciate Shawn's input on it, too. I think rather than put in a
bunch of booleans to alter the behavior of the method, you should just
do the most general thing, and let the caller deal with the result.
For instance, always return the full dict. For non-matching patterns,
perhaps the return dict could map those patterns to None, or empty
lists, or perhaps a second return value would be better. I feel like
raising an exception for these sorts of "problems" isn't really the
right interface. And maybe 'universe' might have more utility as a
catalog object?
I explicitly didn't want to make universe a catalog object. Our catalog
objects are, as far as I can tell, fairly tightly coupled to our on disk
storage and the four pieces of a catalog, etc... Since all we're using
the catalog for here is to produce a list of appropriate packages to
match against, I think providing a way to pass in exactly that is the
right interface.
As it is, what does it mean to set match_type=MATCH_INST_VERSIONS,
not_match_ok=True, and not_inst_ok=False (at least, I think that's one
confusing possibility).
This would be a rather extensive change in areas of the code that are
not well tested by our test suite. (I'm referring to our error
conditions on install/uninstall.) So, I'd much prefer, at this point, to
leave this as is.
- line 2801: I don't understand this. If we're here, not_inst_ok=True
isn't going to prevent an exception. And not_installed will always be
a list. So why zero it out? If the caller simply asked that an
exception not be raised if a pattern didn't match an installed package,
can't they just ignore the missing_matches member of any
PlanCreationException that gets raised?
The short answer is that it provides remarkably confusing output
otherwise, if this list isn't emptied out.
The longer explanation is that the caller, by setting not_inst_ok, is
explicitly declaring that packages which match, but aren't installed,
are ok (ie, aren't an error condition). Emptying the list means that the
caller doesn't have to muck with the exception's internals so that it
matches what they asked for. Sure, the caller CAN empty the list
themself, then reraise the exception, but since they've already said
"Hey, this isn't an error condition" why make them catch, modify, and
rethrow an exception when there's no reason to?
- line 3049: spaces around ==
ok
- line 3053: what happens if there's a wildcard in the version side of
the pattern?
Do mean a pattern like 'foo@*.1.2'? I don't know, I'll check into it.
- line 3064: "remaing" -> "remaining"
thanks
pkg_solver.py:
- line 370: What does an example of this look like? "f" contains the
fmri of what, exactly? The fmri which is frozen?
It looks exactly like all the other solver error messages? Here are some
examples:
pkg install: No matching version of foo can be installed:
Reject: pkg://local/foo@1,5.11-1:20110614T221835Z
Reason: This version is excluded by freeze pkg:/[email protected],5.11.
pkg update: No matching version of foo can be installed:
Reject: pkg://local2/foo@2,5.11-1:20110614T221841Z
Reason: This version is excluded by freeze pkg:/foo@1,5.11.
pkg update: No matching version of foo can be installed:
Reject: pkg://local2/foo@2,5.11-1:20110614T221841Z
Reason: This version is excluded by freeze
pkg:/foo@1,5.11-1:20110614T221835Z.
f is the package name and version to the extent the user specified when
the freeze was executed.
t_pkg_api_install.py:
- line 978: leftover debugging?
No, that's checking that pkg update has the right error code in this
situation.
t_pkg_freeze.py:
- line 141: Ew, why is the ",5.11" in the output? And perhaps a freeze
listing should emit columnar output, rather than a bunch of sentences?
Well it's there since that's part of the version that it's actually
being frozen at, since all fmris have to have their build release set.
But since I've discovered that it's currently impossible to actually
publish (using pkgsend publish at least) a package with a build release
other than 5.11, I don't really care.
I'm fine with columner output. Are you happy with
Name Version
foo 1.2
?
Thanks,
Brock
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
