Allow setting default variants#2644
Conversation
|
Whoo! This should fix #2165. |
|
Might also want to document this. |
|
The documentation actually states this, which made me think that this would/should be supported anyway:
Of course, I could add something more explicit. |
|
Not all packages have a
My larger concern is that this might be jumping the gun on our discussion of variant forwarding vs. universal variants. This features seems akin, somewhat, to universal variants. I think it would be best if we can nail down a coherent design before going ahead with features here and there. |
| """Return a VariantMap of preferred variants and their values""" | ||
| variants = self.preferred.get(pkgname, {}).get('variants', '') | ||
| for pkg in (pkgname, 'all'): | ||
| variants = self.preferred.get(pkg, {}).get('variants', '') |
There was a problem hiding this comment.
would not this add default variant to a package which does not have it? Does it make its hash change? Can you please test it (take any two dependent packages where root has some variant and a dependency does not)?
There was a problem hiding this comment.
This is working correctly as far as I can tell. I have added a dummy variant to m4 and built it with and without an appropriate packages.yaml. The hash of the only dependency does not change and both m4 variants use the same dependency.
There was a problem hiding this comment.
@Suraia: This will fail if the package doesn't support the particular global variant. It sounds like it worked for you because you added a variant to m4, but it would be nice to prefer all: +debug even when some packages don't have debug. I'll put how to fix it below.
|
@citibeth For me, this is not really about the X variant. Maybe someone wants to disable MPI variants globally or just not specify each package individually for a common variant. This only fixes something that should have been working all along in my opinion. |
|
I agree, this PR improves usability.
|
tgamblin
left a comment
There was a problem hiding this comment.
LGTM but I believe this will still fail if a package doesn't support a variant.
| """Return a VariantMap of preferred variants and their values""" | ||
| variants = self.preferred.get(pkgname, {}).get('variants', '') | ||
| for pkg in (pkgname, 'all'): | ||
| variants = self.preferred.get(pkg, {}).get('variants', '') |
There was a problem hiding this comment.
@Suraia: This will fail if the package doesn't support the particular global variant. It sounds like it worked for you because you added a variant to m4, but it would be nice to prefer all: +debug even when some packages don't have debug. I'll put how to fix it below.
| break | ||
| if not isinstance(variants, basestring): | ||
| variants = " ".join(variants) | ||
| return spack.spec.Spec("%s %s" % (pkgname, variants)).variants |
There was a problem hiding this comment.
Can you change this line to look like this:
s = spack.spec.Spec("%s %s" % (pkgname, variants))
pkg = spack.repo.get(pkgname)
return dict((v.name, v) for v in s.variants if v.name in pkg.variants)135879b to
f9f141c
Compare
|
@tgamblin I have actually built a few packages that did not have variants that were specified globally and those did not fail for me. I see your point, though, and have pushed an updated version. |
f9f141c to
0e447b1
Compare
Currently, it seems to not be possible to set default variants for all packages like this:
This PR allows doing so.