retry: bugfix: package requirements with git commits (#35057)

[tgamblin]

Trying #35057 again, as it was broken on develop. I guess the PR was not up to date, and it failed despite CI passing. @scheibelp FYI.

• Specs that define 'new' versions in the require: section need to generate associated facts to indicate that those versions are valid.
• add test to verify success with unknown versions.

[haampie]

To what extent is this pr a workaround for the fact that @x gets interpreted as specific version x versus any x dot something? My pr should solve that for the most part. Maybe good to coordinate those two?

[alalazo]

Failure probably related to #36258

[tgamblin]

@haampie:

To what extent is this pr a workaround for the fact that @x gets interpreted as specific version x versus any x dot something? My pr should solve that for the most part. Maybe good to coordinate those two?

AFAIK this is not a workaround for that at all. I do not think that solves this problem.

[tgamblin]

@scheibelp: I think the main issue with this was the incompatibility with #36258 that @alalazo mentioned. I think that is all that needs to be fixed, but I also think the DeclaredVersion thing above can be simplified back to how it was if we just change the idx -- can you address those two things?

[haampie]

AFAIK this is not a workaround for that at all.

You're right, it's mostly a workaround for git_version == version being true, which is a bug, as it breaks transitivity as well as the equality-implies-identical-hash contract.

[scheibelp]

• I have introduced distinct index/version-provenance everywhere to minimize user confusion about comparisons and eliminated some unnecessary checks in c92938f, and that is sufficient to address all needs of this PR without using an extended definition of DeclaredVersion, BUT...
• DeclaredVersion is about what versions to declare in the solver, which means their comparison has more to do with Version.satisfies than with Version.__eq__
  • For example in pkg_version_rules it constructs a set with sorted(set(self.declared_versions[pkg.name]), key=key_fn)
• This PR respects (and enforces) all described meanings of (and differences between) __eq__ and .satisfies
• Furthermore, the fact that there is no global index generator is a source of fragility that could easily reintroduce bugs into this code (hence I recommend keeping the extended definition of DeclaredVersion)

[haampie]

See above

[tgamblin]

LGTM

[haampie]

This is very bug prone. If no version is imposed by the user, it's looked up and something is assigned. So you can have v1 = abcdef=1.2.3 and v2 = abcdef for which v1.satisfies(v2) but v2 < v1 (as it causes a lookup of ref_version).

I maintain that we should just remove this lookup nonsense, it's just introducing too many edge cases, it's too opaque, and generally useless.

[scheibelp]

I'd rather have a centralized conversation on this (if there is such a conversation, can you link to it here?).

In this particular branch of the if statement, neither version is supplied by the user. So we are comparing things like foo@abcdef... and foo@abcdef..., they are equal if the git IDs are equal as presented by the user.

There is a bug if the lookups would render to the same git ID: (e.g. a commit and a tag) e.g. foo@abcdef... and foo@git.tag1 (if tag1 resolves to abcdef...), although that's separate from what you mention here.

[alalazo]

@tgamblin @scheibelp Sorry if I came late to reviewing this, and previously stopped at reading the title. My understanding was that this PR allowed unknown git versions to be used in package requirements, which is 👍 .

Reading the implementation (while resolving conflicts in other PRs) it seems to me that this PR makes any non-existing version in package requirements an allowed one. I think this is a bug. For example:

packages:
  zlib:
    require: "@2.0" # This version doesn't exist and might be the result of a typo

With the previous behavior on develop we would get:

$ spack spec zlib
==> Error: Cannot satisfy the requirements in packages.yaml for the 'zlib' package. You may want to delete them to proceed with concretization. To check where the requirements are defined run 'spack config blame packages'

which, despite the error message that can be improved, is what I expect by a requirement. This PR made it such that:

$ spack spec zlib
Input spec
--------------------------------
zlib

Concretized
--------------------------------
zlib@2.0%gcc@9.4.0+optimize+pic+shared build_system=makefile arch=linux-ubuntu20.04-icelake

which in my opinion is wrong, since no version 2.0 exists for zlib. Was that on purpose? Shouldn't we restrict this logic to only git versions?

[scheibelp]

This version doesn't exist and might be the result of a typo

which in my opinion is wrong, since no version 2.0 exists for zlib

If the user says it must be zlib@2.0 then I think that's what it should be: if you say spack spec zlib@2.0 on the command line then that's what we get (and Spack tries to find 2.0 from zlib's download link).

If nothing else, right now if a user says require: foo@hash=1.5 and 1.5 doesn't exist in the package.py, then we want Spack to still allow it. I think it's consistent to extend that to all versions mandated in the config.

Printing a warning message might be in order if the package can't be fetched, but otherwise I would say this is the desired behavior. If you want to prevent declaration of number versions that don't exist (e.g. when not associated with a git hash), then that would work for the use cases I created it for, but would be inconsistent (for the reasons outlined above).

[alalazo]

I think it's inconsistent somehow. If the user says +shared we don't allow a shared variant where it doesn't exist. I think the only case where popping a non-git version into existence make sense are externals, since it's software already installed - and somebody put the information on its version in packages.yaml.

[alalazo]

If nothing else, right now if a user says require: foo@hash=1.5 and 1.5 doesn't exist in the package.py, then we want Spack to still allow it.

That's a git version, so I'm not questioning that. It's really "regular" versions that shouldn't be added in my opinion to the allowed declared ones if they appear in requirements.

[scheibelp]

If I do

$ spack spec zlib@2.0
Input spec
--------------------------------
zlib@2.0

Concretized
--------------------------------
zlib@2.0%apple-clang@12.0.0+optimize+pic+shared build_system=makefile arch=darwin-bigsur-skylake

do to make sure, are you also arguing for not doing that anymore?

[Paul-Ferrell]

When we define an environment, we also validate that the concretization output matches all of the versions we specify. So that should, in theory, catch the fact that the defined default in packages doesn't match what we get.

A warning would be nice though, that way we see the discrepancy faster.

Also, having packages define default versions that are undefined in the package causes concretization errors. The version doesn't end up in the concretization facts, version checks in when clauses all fail for that version.

[scheibelp]

@alalazo

But that shouldn't be defining a version 2.0 if it doesn't exist somewhere else.

To be clear, that was the behavior in Spack prior to this PR: putting versions in the preferences defines them as possible versions (i.e. if 2.0 is a version preference, spack spec zlib will give you zlib@2.0). Not to say you can't undo that You can undo that, but it will take (slightly more) than reverting parts of this PR to get what you want.

Therefore

Get back the previous behavior for non-git versions

the "previous behavior" doesn't seem to be a valid given your own stated desires. I suggest outlining the things you want to be true (e.g. as done in #36347 (comment)) and then we can make them true (ignoring however they were behaving in the past).

[alalazo]

To be clear, that was the behavior in Spack prior to this PR [ .. ]

I know, sorry if the reply was confusing but:

1. My immediate worry is if we can get back the old behavior for non-git versions in requirements
2. Then you asked what I thought about the other parts (preferences, CLI) so I gave an answer to that too - but changes there can definitely wait

Also, see this issue we got today #36574 which is related to the discussion.

[scheibelp]

It is related but FWIW if you check out 9a93f22 (the commit before this was merged) you can still reproduce #36574. In some sense, my goal will be to extend this to do the opposite of what was decided there (for hash=number versions at least).

Preventing users from introducing new non-hash versions in preferences will not conflict with this PR or the users I know of that would benefit from it, so I only oppose it on account of it being inconsistent (I am willing to be overridden on that).

[scheibelp]

It is related

I said this because I assumed that package requirements would have the same problem, but they don't: #36589; therefore that issue is not related to this PR (or at least not caused by it).

[alalazo]

Can anybody remind me what is defined_version¹ and explain why it has an hard-coded weight of 1? This is purely for my knowledge and to record the explanation somewhere.

Footnotes

1. I assume it's the =version part of a git version, but want to double check. ↩

[scheibelp]

If you have hash=number then defined_version is the number. It has a hard-coded weight of 1 because it is the next possible weight after 0 (i.e. we add either 1 or 2 items in this "dimension" the first having weight 0, and the next one - if added - has weight 1).

[alalazo]

I don't think this will cause issues right now, but note that this introduces multiple versions with weight 1 in a context where there are multiple versions of weight 0 (and one that is very likely enforced, which is the one above). So, we'll likely have:

0 -> this version
0 -> the default preferred version 
1 -> the defined_version
1 -> the next default preferred version
2 -> ...