I just added a fix for #1557 to this PR. The two bugs fixed are somewhat different... but they're in the same PR because the fixes for them affect the same area of code.

@adamjstewart @davydden @tgamblin @alalazo Looking for review here...

One goal here is to do away with special cases inside of Spack for @system, @develop, etc. I do use @develop versions in my work; but they are tied to the @develop branch in my code's GitHub repo.

I've concluded that one grand Version comparison to rule them all is not really appropriate for us (because the correct version to choose in concretize.py depends on just on the versions themselves, but also what the user put in packages.yaml). I think the code I've put into concretize.py is easier to follow (and more correct) than what was there before. Are there other places where we compare version numbers? Or is all the Version.lt() machinery just there for concretize.py?

Some documentation changes will be required for this PR (just a short paragraph). Since this PR touches some possibly controversial issues, I'd like to hold off on docs until we all like what we see.

One problem here is, what happens when a system-supplied version does not exist in Spack? For example... I'm unable to build Python2 for some unknown reason. But I have a system-installed Python2. And I CAN (and need to) build Python3. So I've put in packages.yaml:

packages:
    python:
        paths:
            [email protected]: /        # Spack can't install Python2, I don't know why
        version: [3.5.2,2.7.5]

Now consider: spack spec python@:2.8. Unfortunately, Python's package.py file only goes back to version 2.7.8. In that case, the concretization code I put in this PR essentially throws away version 2.7.5, leaving the user with version 2.7.12.

There are workarounds to this, of course. But do you think we should extend the concretization code to look for versions specified in the paths: attribute of packages.yaml?

One goal here is to do away with special cases inside of Spack for @develop

disagree. As stated previously @develop has a perfectly fine meaning of being the bleeding edge. It is not just an extra tag, it also helps writing depends_on() and actually allows to write complicated dependencies which are not possible without. See this PR (especially OP) and discussion there #1190 together with the unit tests. We collectively agreed on this feature and I really don't like it to be removed!

p.s. @develop is a version of a package from the official master branch of developer's version control system (Git, SVN, etc). It is not primarily for developers, but rather for users who want to have the bleeding edge functionality and/or bugfixes. That's why @develop need to play nicely in version comparison and depends_on().

Denis,

I see we did collectively agree on develop, and I actively participated
in the thread, thank you for catching this.

HOWEVER... the code checking for develop in concretize.py was causing
problems. By always throwing away the develop version, it breaks the
following in my packages.yaml file --- which I use as the developer of
ibmisc:

    ibmisc:
        version: [develop]
        variants: +python +netcdf

Therefore, I am suggesting (below) ways to maintain what we agreed on in
#1190 while enabling the use of develop in packages.yaml.

Also, thinking it through again... what if I have more than one branches,
all of which are development versions? I would suggest extending the
develop functionality in version.py so that any version starting with
develop. will be considered a development version.

Putting it all together, I'm suggesting the following code changes:

1. Add an isdevelop() method to version.py, like the isnumeric() I
   just added.
2. Change version.py (essentially) back to what it was, with the
   following precedence, newest-to-oldest: isdevelop(), isnumeric(),
   . Within each group, versions will be sorted alphabetically
   or numerically.
3. Note that concretize.py specifies a different sort order than
   version.py (it always has, but that's more explicit and flexible now).
   In concretize.py, the sort order should be the following instead:
   isnumeric(), isdevelop(), . This will replace the following
   code, while still allowing users to specify develop version in
   packages.yaml:

+ # Disregard @develop and take the next valid version
+ if ver(valid_versions[0]) == ver('develop') and len(valid_versions) > 1:
+ spec.versions = ver([valid_versions[1]])
+ else:
+ spec.versions = ver([valid_versions[0]])

1. Update tests and docs that were updated in add special treatment of @develop version #1190.

Any thoughts?
-- Elizabeth

Therefore, I am suggesting (below) ways to maintain what we agreed on in #1190 while enabling the use of develop in packages.yaml.

I am fine with any solution which keep the unit tests in #1190 working:

self.assert_ver_eq('develop', 'develop')
self.assert_ver_lt('1.0', 'develop')
self.assert_ver_gt('develop', '1.0')

(current state of this PR breaks these tests).
Plus conretize shouldn't fall to @develop by default when running spack install <package>.

Also, thinking it through again... what if I have more than one branches,
all of which are development versions? I would suggest extending the
develop functionality in version.py so that any version starting with
develop. will be considered a development version.

Makes perfect sense to me, although i most likely won't be using it myself.

Putting it all together, I'm suggesting the following code changes:

That also make sense to me.

@tgamblin @davydden

I am fine with any solution which keep the unit tests in #1190 working:

self.assert_ver_eq('develop', 'develop')
self.assert_ver_lt('1.0', 'develop')
self.assert_ver_gt('develop', '1.0')

Thinking it through again, why do we need this?

If develop>numeric, and develop versions aren't to be released to users, that means we can't merge any packages with a @develop version in them. The workflow then becomes:

1. Add a @develop version while developing a package.
2. Remove @develop just before you make a PR.
3. Add @develop back every time you want to keep working on the package.

Sounds like a pain to me. How about an alternative workflow, based on the simpler idea that numeric versions are always greater than non-numeric versions:

1. Add a @develop version and leave it in the package permanently. This allows users to access it if they want, but Spack will never access it by default.
2. Specify @develop in your packages.yaml while you are developing this package. For example, I use:

    ibmisc:
        version: [develop]
        variants: +python +netcdf

1. Release the same package.py that you used to develop with. Comment out the entry in your packages.yaml when you're done developing.

Can you explain a situation where the first workflow would be preferable to the second? I just can't convince myself that this special case for develop>numeric is every really the best thing.

Another thought (that might counteract what I just said above): Spack doesn't have, but really needs, fail-safe protection on trusted vs. untrusted download. A download is trusted, basically if it could be checksummed or otherwise verified. In practice, that means that tarball and Git hash downloads are trusted, whereas Git branch or tag downloads are not.

By default, Spack really really should only install packages from trusted downloads. In practice, that would mean that Spack would typically ignore @develop versions no matter what their sort order is.

If develop>numeric, and develop versions aren't to be released to users

This is exactly what I would avoid by all means. Users sometimes need to compile a @develop version of a package and use it for production runs because they know that some crucial bug was fixed.

Here's a real life example1:
John is a PostDoc and uses packageA for his production runs. While working on his C++ code he found a bug in packageA. He contributed a PR upstream with a unit test to fix a bug. John is not a developer of packageA, he is a mere user. After fixing it upstream, he want to go back to his C++ code and use @develop version of packageA, which has a critical for him bug fix.

Here's another example:
John knows that some important for him feature was just added upstream to packageA. He needs to use this feature in his production runs but he can not afford to wait until the next release. Thus, he needs to install @develop version of packageA.

Let us not limit the users here.

Myself, with a user's hat of Spack, I use @develop version all the time for my production scientific runs. Myself, with a developer's hat of some package, I never use @develop version because I develop using Filesystem views.

I think the confusion here was that @develop does not mean this is for developers or use this to develop package A... (you can do both, though). It only means that this is the current upstream @develop version of a package. And it is a perfectly fine wish/need of users to compile and employ it in production for reasons outlined above.

I hope this clears things out a bit.

p.s. I did minor edits to expand on examples.

By default, Spack really really should only install packages from trusted downloads. In practice, that would mean that Spack would typically ignore @develop versions no matter what their sort order is.

correct. The behaviour you describe is exactly how it works now in Spack. spack install package will install the highest release version (i.e. 11.2.3).

Whoah, let's go back to the beginning on this PR... As I mentioned in #1696, I think it will be best to table the discussion on @develop as a special case.

The original purpose of this PR was to fix bugs that were causing not one but two serious problems ( #1556 and #1557 ). These bugs popped up as soon as I tried to use packages.yaml in a meaningful way. Use of packages.yaml was required because needed to use a version of MPI installed by our sysadmins on our supercomputer (because they compiled with magic, and unknown, options that work with their fancy interconnect hardware). There really were no workarounds here. So I had to dig down and do SOMETHING to make packages.yaml work.

As I dug into what was going on, I discovered that the bugs were caused by a series of "hacks" applied to the logic that chooses which version to install, without anyone really thinking it through all the way. Each time a hack was applied, it probably solved problems for one specific use case; but ultimately ended up creating brittle, unpredictable code.

For that reason, we can't just leave it alone. To get this right, we absolutely need clear thinking on how Spack selects which version to install; and special cases frequently work against clear thinking. I suggest the best way forward for now is to make the current PR conform to the unit tests of #1190. I don't think special cases are a good idea, or even required. But I would rather continue to argue my case on a non-buggy version of Spack.

I suggest the best way forward for now is to make the current PR conform to the unit tests of #1190.

I never argued otherwise, as I said above

I am fine with any solution which keep the unit tests in #1190 working:

self.assert_ver_eq('develop', 'develop')
self.assert_ver_lt('1.0', 'develop')
self.assert_ver_gt('develop', '1.0')

You asked

Thinking it through again, why do we need this?

that's why I gave a few examples of when users need @develop to be treated right in version comparison in Spack and be released to users. Version comparison is important here, if it does not work, it's impossible to use @develop in packages.

that's why I gave a few examples of when users need @develop to be treated right in version comparison in Spack and be released to users.

I read your examples, and can respond to them later. Long story short, once we have trusted downloads, I think I have a scheme that will serve your use cases while keeping Spack's package selection rules as simple as possible. And I think I can convince you that it works for your use cases.

@tgamblin @davydden This is ready to merge:

• It passes the unit tests that @davydden brought up. I want to re-open those tests, but not now. This needs to get merged.
• It passes flake8
• It fixes my bugs.

@citibeth: can you update the docs based on this?

OK, see #1933

We should get this merged today, if possible; when looking over the PR
again, I found a significant bug that is now fixed in #1933 above.

On Thu, Oct 6, 2016 at 5:35 AM, Todd Gamblin [email protected]
wrote:

@citibeth https://github.com/citibeth: can you update the docs based on
this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1561 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1cd4pm6Ji5PXVgaB0XzBwxV2M5gYptks5qxMD0gaJpZM4JoHFY
.