On 05/31/12 18:30, Tim Foster wrote:
On 06/ 1/12 10:29 AM, Brock Pytlik wrote:
https://cr.opensolaris.org/action/browse/pkg/timf/pkgdep-smf-req-any
[snip]
54,63: There needs to be an else clause to this if/elif, or we need to
set default values for base_names and paths I think.
I've added that, though it seems weird that we'd create an empty
SMFManifestDependency that doesn't specify either a string or a tuple
for a path?
I agree, personally I'd throw an exception if we don't get a string or
tuple.
307: nit Could you change line 307 so that it says "service FMRI, not an
instance FMRI, ..."?
Sure.
310-316: I think this error message could be a little clearer and nicer
to the user. First, since manifests is a list, the error message the
user sees is going to see the python representation of a list ie: '[foo,
bar, baz ...]' without any linebreaks. While the idea of using locals()
like this is cute, i think it often isn't what we want because it
doesn't format the error info nicely for the user. The colon on line 314
also seems a bit out of place to me, at least with how I'm reading it in
my head. Perhaps a period and then start another sentence? I think it's
also worth explaining to the user why multiple files can't satisfy a
dependency in this situation since they're allowed in other situations.
Fair comment. As 309 says, this should never happen, so I'm not hugely
worried about the format of the exception message, but I have reworded
it, removed locals(), and tried to explain the error a bit more
It now says:
"_(Unable to generate SMF dependency on the service FMRI %(dep_fmri)s
declared in %(proto_file)s by %(fmri)s. SMF dependencies should always
resolve to SMF instances rather than SMF services and multiple files
deliver instances of this service: %(manifests)s") %
{"dep_fmri" : dep_fmri,
"proto_file": proto_file,
"fmri": fmri,
"manifests": ", ".join(manifests)})
though I don't know if that makes things much clearer.
I think that helps us as maintainers at least :) Thanks for the change.
Testing:
What happens if a file that a service depends on is delivered under only
1 of 2 variant values?
Here's an example dependency that these bits generate (no variants are
specified in this particular case)
depend fmri=__TBD \
pkg.debug.depend.fullpath=lib/svc/manifest/rsyslog.xml \
pkg.debug.depend.fullpath=lib/svc/manifest/system-log.xml \
pkg.debug.depend.reason=lib/svc/manifest/ipmon.xml \
pkg.debug.depend.type=smf_manifest type=require
we leave it up to "pkgdepend resolve" to do the right thing with this
dependency, and that behaviour isn't modified by these bits.
If rsyslog.xml and system-log.xml have different variants (say one's
delivered to zones and the other isn't), then pkgdepend resolve gets us:
depend fmri=pkg:/[email protected] type=require
variant.opensolaris.zone=global
depend fmri=pkg:/[email protected] type=require
variant.opensolaris.zone=nonglobal
If rsyslog.xml and system-log.xml only deliver to the global zone, but
our package delivers to both zones and global zones we get a
resolution failure:
/home/timf/projects/ips/pkgdep-smf-req-any-pkg.hg/testdata/file.mf.dep
has unresolved dependency '
depend type=require fmri=__TBD \
pkg.debug.depend.reason=lib/svc/manifest/ipmon.xml \
pkg.debug.depend.type=smf_manifest \
pkg.debug.depend.fullpath=lib/svc/manifest/rsyslog.xml \
pkg.debug.depend.fullpath=lib/svc/manifest/system-log.xml
' under the following combinations of variants:
variant.opensolaris.zone:nonglobal
Now, there is an edge-case which we aren't handling properly.
If rsyslog.xml delivers to a zone and system-log.xml delivers to both
global and non-global zones, but the rest of the rsyslog package can
appear in a global zone, then we get:
depend fmri=pkg:/[email protected] type=require
variant.opensolaris.zone=nonglobal
depend fmri=pkg:/[email protected] fmri=pkg:/[email protected] type=require-any
variant.opensolaris.zone=global
That last require-any dependency is wrong, if rsyslog was installed in
the global zone (without the SMF manifest) and old-syslog wasn't, then
we've incorrectly satisfied the dependency.
Ultimately, I know we want to move SMF fmri->file resolution into the
"pkgdepend resolve", which might help us here, but that's not this
changeset.
As far as I can tell, we should already have this broken behaviour in
the gate for other dependency types that deliver to multiple
locations, but split their content across variants (we just don't have
any instances of it occurring in the real world yet)
Huh, I guess I haven't tested for this case. I'm a bit surprised it
behaves (on other forms of dependencies) as you've described, but since
I haven't tested it ...
I'll look at fixing that up then and hopefully it won't get too painful.
Brock
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
