On 06/ 1/12 10:29 AM, Brock Pytlik wrote:
https://cr.opensolaris.org/action/browse/pkg/timf/pkgdep-smf-req-any
Thanks for taking a look,
smf_manifest.py:
58-62: nit: could this comment be located inside the elif block? Maybe
it's just a personal peeve of mine, but having the comments where they
are makes the code hard to read to my eyes.
Sure, I've fixed that.
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?
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.
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)
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
