Brock Pytlik wrote: > >>http://cr.opensolaris.org/~bpytlik/ips-18441-v1/ > > > >client.py: > > > > - 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.
Hm. I probably would drop the message entirely if it did what you asked. > >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? Maybe just when you list freezes? The solver's failure output is large enough, I don't think this would add anything useful. I don't really care about the output format. > > - 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. I'm thinking about a situation where you're operating on an image that has a newer version than you support. True, you might not be able to do anything, but stack tracing isn't a great answer. Asserts should be used to confirm invariants in your program, not handle out of scope scenarios. Okay. Though that was really not the focal point of the comment. > > 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. Okay. I hate for code to get more gross because we don't have time to factor it out in a sane fashion. Given the time pressure we have now, if someone gets exasperated with the factoring here, they can fix it. > >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. But "pkg:/[email protected],5.11" isn't a freeze, it's a package FMRI. Perhaps this just needs to be This version is excluded because of a freeze on <stem> at version <version> > >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. If it's not part of what the user asked for, it shouldn't be in the output. > I'm fine with columner output. Are you happy with > Name Version > foo 1.2 All caps for the column headers. And add -H, and -F tsv/csv. Hopefully there's code for you to reuse for this. Thanks, Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
