Thanks! I'll get a chance to look at this in an hour or so

After looking into this IMO it would be more suitable to change the extensions command by passing in a more general spec to the repo.extensions_for call.

When calling spack extensions X there are two phases:

1. Spack reports all possible packages that could extend X
2. Spack reports all currently activated extensions of X

For some reason, right now on phase 1, spack is querying for an installed spec to find out all possible packages which could extend the specified package. All it needs for that is the package name (e.g. Spec("python") in this case).

I should say that while the change in this PR wouldn't cause any bugs, it is counterintuitive because it is somewhat conflicting with the rest of the logic in Package.extends, which goes through the effort to retrieve the actual extended dependency if the spec is concrete (so at the moment if you give a concrete package to Package.extends and it returns true then it means not only does it e.g. extend python but it actually extends that particular instance of python).

Does that seem sensible (or raise questions)? Are there other issues coming up that this change would help with?

The augmented arguments for spack extensions mentioned in #5415 (comment) sound somewhat in sync with my reasoning at #5600 (comment) - do you agree?

For some reason, right now on phase 1, spack is querying for an installed spec to find out all possible packages which could extend the specified package. All it needs for that is the package name (e.g. Spec("python") in this case).

I expect this is historical. The problem is that I have two Pythons installed, [email protected] and [email protected] and I have to disambiguate right now. If only doing -s packages, the disambiguate step can be skipped.

I should say that while the change in this PR wouldn't cause any bugs, it is counterintuitive because it is somewhat conflicting with the rest of the logic in Package.extends, which goes through the effort to retrieve the actual extended dependency if the spec is concrete (so at the moment if you give a concrete package to Package.extends and it returns true then it means not only does it e.g. extend python but it actually extends that particular instance of python).

Well, currently it returns False if I give it a concrete spec even though I know have have installed extensions. Why would this fix it?

Well, currently it returns False if I give it a concrete spec even though I know have have installed extensions. Why would this fix it?

To be clear, are they also activated? Right now the logic should only pick up activated extensions. This is another confusing aspect of spack extensions because it talks about installed extensions when it is actually reporting activated extensions.

I fixed that part in #5415 as well; it now reports installed extensions as well. And yes, even if they're activated, they're not detected unless this patch is applied.

I fixed that part in #5415 as well; it now reports installed extensions as well. And yes, even if they're activated, they're not detected unless this patch is applied.

I'll pull your branch and check it out.

At the moment I'm having trouble replicating this error with your branch: e.g. spack extensions --show=installed python shows me what I'd expect. Is it that --show=installed is working but --show=activated is not? I'll have another chance to look into this in a few hours.

EDIT: to be clear I meant to say that #5415 appears to be working using --show=installed without applying the patch in this PR

Oh, so this happens:

% bin/spack extensions [email protected]
==> [email protected]%[email protected] patches=123082ab3483ded78e86d7c809e98a804b3465b4683c96bd79a2fd799f572244 +pic+shared~tk~ucs4 arch=linux-fedora25-x86_64 /hewrszv has no extensions.
% bin/spack extensions --show=installed [email protected]
==> 2 installed:
-- linux-fedora25-x86_64 / [email protected] ----------------------------
[email protected]  [email protected]
% bin/spack extensions --show=activated [email protected]
==> None activated.

So the "available packages" query doesn't work and then bails early thinking "well, there couldn't possibly be any installations which match".

Even with my other PR, this seems to be necessary. Why, if I request the extensions of a certain version of a package, does the list of available extensions return nothing, but I have some installed? The commit in #5415 makes it so you can skip the "oh, there's no possible package that extends it, there couldn't possibly be any installed" logic, but the underlying problem still exists.

Also, this makes more sense in the sense that it now says "if the spec you asked to list extensions for satisfies the spec given in this package, this package extends the spec you gave". The other way reads backwards. Or is the method itself backwards?

IMO the method Package.extends is OK; the issue results from the combination of how it works and how it's called. My argument is that it works fine and should be called differently. For example Repository.extensions_for could create a spec with just the name, or spack extensions could call extensions_for with just the name of the spec. I'm not sure what you mean when you say "the other way reads backwards". I should also say it is confusing to me to think about how Package.extends should behave in terms of producing the right output when called indirectly from the spack extensions command.

Why, if I request the extensions of a certain version of a package, does the list of available extensions return nothing, but I have some installed?

At the moment (i.e. without the changes in this PR) Package.extends requires that the extendee of the package matches all the details of the provided package. Repo.extensions_for is taking each generic package and checking to see if it extends a specific provided spec, which fails because the extendee of the generic package is general.

Does that seem sensible?

My argument is that it works fine and should be called differently.

How? It takes a single argument.

For example Repository.extensions_for could create a spec with just the name

That doesn't seem right. Packages could say they extend only python@3:. Just a name isn't going to match that.

or spack extensions could call extensions_for with just the name of the spec

Maybe for available, yes. But if I ask about Python2, I should not see any Python3-only packages (and vice versa), so the version has to be there in the general case.

I'm not sure what you mean when you say "the other way reads backwards".

a.satisfies(b) says, to me, that a satisfies the requirements given in b. The current code is saying the spec I extend satisfies the requirements you have given. My code says the spec you have provide satisfies what I require. The second sounds more like the logic of what we want here: "Does the spec I have asked about satisfy the requirements of what you extend?".

Evidence that the satisfies argument should be more "generic". From lib/spack/spack/test/concretize.py:291:

assert spec['libdwarf'].compiler.satisfies('clang')

The argument I give the extensions plays the role of "what I have" which is the compiler bit here.

OK after thinking on it more I think the order you have set here is fine (and e.g. handles the extends(python@3 case as you mention). I think it would be useful to add a comment explaining the semantics of the method:

If self and spec are concrete, this reports whether self is an extension of param. Otherwise this reports whether self could extend param (given whatever is specified for param).

IMO that is worthwhile because this is called in a few contexts and some of the callers (e.g. Database.installed_extensions_for) want to know more than whether the package could extend the provided spec.

If self and spec are concrete, this reports whether self is an extension of param. Otherwise this reports whether self could extend param (given whatever is specified for param).

self can't be concrete; it is a package.

self can't be concrete; it is a package.

I think it would be accurate to change the text to "if self.spec and spec are concrete..."

Regarding the added method description:

Returns True if this package may extend the given spec.

Database.installed_extensions_for uses this and needs a stronger guarantee than whether a package may extend a given spec. This function provides that guarantee but only under the circumstances I mentioned. Would the description I suggested be suitable given your correction? I think it captures all the needed behavior.

I think it would be accurate to change the text to "if self.spec and spec are concrete..."

Hmm. Package has-a Spec seems odd to me, but OK. Docstring update incoming.

Oddly this is now failing the flake checks on account of bare except statements (which are not affected by this PR). I didn't see any recent changes to spack/.flake8 so perhaps the flake version used by travis changed. I added a PR at #5896 to address that, at which point the tests should pass.

I added a PR at #5896 to address that, at which point the tests should pass.

This is now merged and I think if I close & reopen this it will test with this rebased on develop so I'm going to start that.

OK really sorry but I thought about this more and the method description still isn't 100% accurate (which is my fault because I suggested it). So the following is precise:

If the spec associated with this package is not concrete, reports whether the package could extend the given spec. If the associated spec is concrete, this only reports whether the package does extend the given spec.

This is slightly different from the method description added in this PR because it won't report whether the package could extend a given spec if the spec associated with the package is concrete. The intuitive meaning of could extend is that if the given spec is not concrete, the package could extend it as long as no details of the package's associated spec conflict with it. In this PR, if the package is concrete but the given spec is not, it will always report False.

While the implementation in this PR is suitable for all the cases where Package.extends is used, a less confusing implementation would be to add a could_extend method that tries if X.satisfies(Y) or Y.satisfies(X), and then Repository.extensions_for would use that instead of Package.extends.

Does that make sense and seem agreeable? Another option is to update the method description for Package.extends but I think adding a could_extend method would make things less confusing in the future.

Yes, a less-confusing set of methods would be nice. However, I think that stems from Package.spec existing at all and I'd like to see that resolved at some point.

There's also the issue that all of this is working only on the first extendee declaration in the package (see the implementation of extendee_spec). I feel like this is something worthy of a separate issue to tackle in the future, not in this PR.