-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow setting default variants #2644
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. |
@@ -154,7 +154,10 @@ def spec_has_preferred_provider(self, pkgname, provider_str): | |||
|
|||
def spec_preferred_variants(self, pkgname): | |||
"""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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, thanks for checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I believe this will still fail if a package doesn't support a variant.
@@ -154,7 +154,10 @@ def spec_has_preferred_provider(self, pkgname, provider_str): | |||
|
|||
def spec_preferred_variants(self, pkgname): | |||
"""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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
for pkg in (pkgname, 'all'): | ||
variants = self.preferred.get(pkg, {}).get('variants', '') | ||
if variants: | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.