Skip to content

Remove url_for_version as it breaks spack info#1503

Merged
tgamblin merged 1 commit intospack:developfrom
adamjstewart:fixes/openssl
Sep 20, 2016
Merged

Remove url_for_version as it breaks spack info#1503
tgamblin merged 1 commit intospack:developfrom
adamjstewart:fixes/openssl

Conversation

@adamjstewart
Copy link
Copy Markdown
Member

Is this @system version even necessary? As far as I know, system installations are handled through packages.yaml. Leaving it in caused this problem:

$ spack info openssl
Package:    openssl
Homepage:   http://www.openssl.org

Safe versions:  
Traceback (most recent call last):
  File "/soft/spack-0.9.1/bin/spack", line 184, in <module>
    main()
  File "/soft/spack-0.9.1/bin/spack", line 161, in main
    return_val = command(parser, args)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/cmd/info.py", line 114, in info
    print_text_info(pkg)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/cmd/info.py", line 62, in print_text_info
    f = fs.for_package_version(pkg, v)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/fetch_strategy.py", line 831, in for_package_version
    attrs['url'] = pkg.url_for_version(version)
  File "/blues/gpfs/home/software/spack-0.9.1/var/spack/repos/builtin/packages/openssl/package.py", line 55, in url_for_version
    return super(Openssl, self).url_for_version(self.version)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/package.py", line 430, in version
    raise ValueError("Can only get of package with concrete version.")
ValueError: Can only get of package with concrete version.

@adamjstewart
Copy link
Copy Markdown
Member Author

@citibeth discovered another problem with url_for_version that breaks spack spec. This PR should resolve #1555.

@davydden
Copy link
Copy Markdown
Member

davydden commented Aug 18, 2016

Is this @System version even necessary?

👍 I think we should not be any system around. Only normal versions and develop so that constraints on packages work. Otherwise we can't tell the version of system provided package and thereby use it properly conretization.

@citibeth
Copy link
Copy Markdown
Member

Otherwise we can't tell the version of system provided package and thereby use it properly concretization.

The same issue applies whether your version is @system or @develop. It's not the end of the world. If users choose to work that way, they will know that Spack's ability to concretize might be compromised in some ways. Since @develop versions should only be installed by the developer (or a close collaborator), this is not a serious problem.

I think we should not be any @system around

We've been over this before, @system is not for every situation. In fact, it is mostly for OpenSSL, which has unusual properties:

  1. It is security-sensitive and used to guarantee the integrity of much of the rest of the system. If you use an OpenSSL installed in user space (as happens with Spack's OpenSSL), then you lose most of your security properties. Therefore, there is good reason to NOT let Spack build OpenSSL. A well-matinained system will upgrade OpenSSL from time to time, for security reasons that the typical Spack user is oblivious to (Heartbleed, anyone)?
  2. OpenSSL updates pretty much never change the API; many don't even change binary API. Therefore, it is not important to Spack users which particular version of OpenSSL is being used.

Anyway, we've been over this before. Please do not remove/break @system. Let's just make it work.

@davydden
Copy link
Copy Markdown
Member

The same issue applies whether your version is @System or @develop.

nope, @develop is always greater than any specific version:
develop will satisfy any @x.y.z: as it is by definition the current state of the code which is ahead of any previously released version. On the contrary, it will not satisfy @x.y.z or @:x.y.z, which is also correct.

@davydden
Copy link
Copy Markdown
Member

Please do not remove/break @System. Let's just make it work.

why can't one say that Openssl is provided externally and instead of a @system give it some version, on my macOS i see

$ openssl version
OpenSSL 0.9.8zh 14 Jan 2016

so i would put 0.9.8zh as a version instead of @system.

@tgamblin
Copy link
Copy Markdown
Member

@davydden: I think @citibeth wants a sentinel to indicate that OpenSSL might be upgraded silently. If you set it to 0.9.8zh, the system can still upgrade OpenSSL. But at least you know what it was built with...

I don't think the problem here has much to do with system being special -- I tried to preserve the existing logic in the OpenSSL package but apparently didn't do it right. I think @alalazo should weigh in here as I think system actually comes from him.

@adamjstewart
Copy link
Copy Markdown
Member Author

@tgamblin I get why an un-versioned system version would be useful, but I agree with @davydden that it breaks a lot of Spack's expectations. @system does nothing more than tell the user that the version may change. If the user is forced to assign an arbitrary version to it, it still works fine, even if the OS installed version changes. If we really want the ability to have a @system version, I think the logic should be elsewhere so that it works for all packages.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Aug 18, 2016

system was a sentinel that served 2 purposes :

  1. avoid the delay due to the old implementation of url_for_version in openssl
  2. permit to have a unique, reproducible hash on a possibly moving target

2 is now the only reason why it should remain.

If the user is forced to assign an arbitrary version to it, it still works fine, even if the OS installed version changes.

As I really like the principle of least astonishment I would prefer a moving target to be tagged rather than having spack reporting a version that may be wrong due to a system update

@citibeth
Copy link
Copy Markdown
Member

why can't one say that Openssl is provided externally and instead of a @System give it some version

Because:

  1. I don't want to think about it, and I don't think I should have to think about it.
  2. Our sysadmins might upgrade my OpenSSL version at any time, and then whatever I told Spack would be wrong.
  3. If I'm allowed to use @system for openssl, I can put @system in a per-project packages.yaml file and be done with it. If I'm forced to specify a numeric version, then I'll have to tell my users to fish out the version of OpenSSL installed on their system and insert that into the per-project packages.yaml file I've provided. This is added complication for no benefit (that I can see).

If you set it to 0.9.8zh, the system can still upgrade OpenSSL. But at least you know what it was built with...

We already know what it was built with, even without any effort on Spack's part, through the use of ldd. Modern Linux systems are managed in a way to ensure that compiled binaries continue to work, even after the libraries they depend upon are upgraded. If that is not the case, it would be because of a security problem that could not be fixed with a new binary-compatible shared library. But I'm not aware of that ever happengin.

I just don't think there's a problem here that needs to be solved.

If we really want the ability to have a @System version, I think the logic should be elsewhere so that it works for all packages.

The convention does work for all packages. Just put the @system and nobuild stuff in your packages.yaml and you're set to go. I've used it for qt as well.

Checking for @system in url_for_version() serves no purpose (that I'm aware of). Even if url_for_version() returns a crazy URL, it will never be fetched as long as build=False.

system was a sentinel that served 2 purposes :

  1. avoid the delay due to the old implementation of url_for_version in openssl
  2. permit to have a unique, reproducible hash on a possibly moving target

I believe I'm the originator of @system, and this is not the purpose. The purpose is to tell Spack that you've chosen to rely on a system library --- and that you believe the library is stable enough (from Spack's point of view) that Spack doesn't have to know the version at compile time. I've used @system in the following cases:

  1. openssl: To avoid having a Spack-installed OpenSSL, which would be a security risk.
  2. qt: Because I didn't feel like coaxing Spack to build Qt at the time. And I felt I'd get the best result if my Spack-built binaries used the same Qt as the rest of GUI apps on my system.

If Spack needs to invent a version number, it should do the same as @develop --- i.e. assume that @system is newer than any numbered version. I would suggest that Spack creates a deterministic ordering of versions for all non-numeric version numbers. Making them all newer than the numbered versions, and then sorting in alphabetical order, would be a decent choice. If this ever breaks for someone, they can always specify the actual version number as others have proposed above.

@davydden
Copy link
Copy Markdown
Member

davydden commented Aug 19, 2016

Ok, let me summarise why i think there should be no @system answering some of those concerns and also propose an alternative solution to those of you who like @system:

  1. When you use @system it essentially breaks all version constrains in Spack turning it into Homebrew/Hashdist/etc. Should @system satisfy @:4 or @5:? The behaviour is simply undefined and one can't/shouldn't come up with any logic here! This may not be important for openssl, but it is important for a general package, including qt. That's certainly against the principle of less astonishment.
  2. System version updates: Say you mark a system-provided package with a proper version and build against it. If it is used as a build only dependency, hidden updates would not break current Spack stack. If it is link/run dependency, then a major hidden update (i.e. qt4 -> qt5) will most likely break everything that is build with the package anyway. So the fact that we marked it as @system does not help here at all. Finally, if it was a minor update (i.e. qt4.0.1->qt4.0.2), most likely it won't break anything, including packages that link against it. My point is -- system-provided packages are dangerous by definition and may lead to un-reproducable builds. Having @system does not solve these problems.

If you really want to distinguish between using system-provided and non-system-provided packages, I believe a better way is to use the version suffix in packages.yaml, i.e. 4.5.3.system for a system-provided package of version 4.5.3. That way all version comparison should work (huray, it's Spack'y again! 😄) and one can also distinguish between packages build against 4.5.3 and unstable-rundomly-updated-by-admin-system-provided 4.5.3.system.

p.s. I guess it's clear from (1) that I am strongly against

it should do the same as @develop --- i.e. assume that @System is newer than any numbered version

p.p.s. obviously naming convention (4.5.3.system or 4.5.3.sys) is up to the Spack user.

@citibeth
Copy link
Copy Markdown
Member

If you don't like it, then don't use it. You admit it will work for SSL.
And sometimes for Qt: Linux systems don't push major upgrades of Qt for the
reasons you mentioned.

We should not have to encode special cases for system, develop, etc.
Current special cases should be evaluated for something general.
On Aug 19, 2016 12:31 AM, "Denis Davydov" notifications@github.com wrote:

Ok, let me summarise why i think there should be no system answering some
of those concerns and also propose an alternative solution to those of you
who like @System:

When you use @System it essentially breaks all version constrains in
Spack turning it into Homebrew/Hashdist/etc. Should @System satisfy @:4
or @5:? The behaviour is simply undefined and one can't/shouldn't
come up with any logic here
. This may not be important for openssl,
but it is important for a general package, including qt. That's
certainly against the principle of less astonishment.
2.

System version updates: Say you mark a system-provided package with a
proper version and build against it. If it is used as a build only
dependency hidden updates would not break current Spack stack. If it is
link/run dependency, then a major hidden update (i.e. qt4 -> qt5) will
most likely break everything that is build with the package anyway. So the
fact that we marked it as @System does not help here at all. Finally,
if it was a minor update (i.e. qt4.0.1->qt4.0.2), most likely it won't
break anything, including packages that link against it. My point is --
system-provided packages are dangerous by definition and which may lead to
un-reproducable builds. Having @System and possible a special
treatment of it does not help.

If you really want to distinguish between using system-provided and
non-system-provided packages, I believe a better way is to use the version
suffix in packages.yaml, i.e. 4.5.3.system for a system-provided package
of version 4.5.3. That way all version comparison should work (huray,
it's Spack'y again! 😄) and one can also distinguish between packages
build against 4.5.3 and unstable-rundomly-updated-by-admin-system-provided
4.5.3.system.

p.s. I guess it's clear from (1) that I am strongly against

it should do the same as @develop --- i.e. assume that @System
https://github.com/system is newer than any numbered version


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

@davydden
Copy link
Copy Markdown
Member

If you don't like it, then don't use it.

it's not about that, it is about Spack not having anywhere in its internals any special treatment of @system, which is exactly what this PR is about.

@citibeth
Copy link
Copy Markdown
Member

On Fri, Aug 19, 2016 at 7:37 AM, Denis Davydov notifications@github.com
wrote:

If you don't like it, then don't use it.

it's not about that, it is about Spack not having anywhere in its
internals any special treatment of @System, which is exactly what this PR
is about.

So I think we agree ;-)

A little history on this PR... at one point, openssl would check on-line
for newer versions every time you did anything (for example, spack spec). This caused numerous problems, as will always be the case when one
goes to the Internet outside of Spack's download or resource mechanism.

One of those problems was that Spack would provide an "out of date" warning
all over the place, if you are using @system. Apparently, what we saw in
url_for_version() here was the fix for that. In this case, the cure was
worse than the disease!

Due to other problems (like... it breaks on nodes that can't access the
Internet), the version checking was removed from openssl altogether.
url_for_version() should have been removed along with it.

@adamjstewart
Copy link
Copy Markdown
Member Author

Checking for @system in url_for_version() serves no purpose (that I'm aware of). Even if url_for_version() returns a crazy URL, it will never be fetched as long as build=False.

Sooo... are we all in agreement that removing url_for_version is the right thing to do here? It solves the issue with spack info openssl and solves #1555, and as far as I can tell has no adverse consequences. If that's the case, let's get this PR merged!

@adamjstewart
Copy link
Copy Markdown
Member Author

For the record, I agree with @citibeth that @system is very useful for packages like OpenSSL where the API is pretty stable at this point (no current packages depend on a certain version). I also agree with @davydden that @system creates a lot of potential problems for packages like qt where the API has changed drastically between versions and many current packages depend on qt@5: or qt@:4. But I also agree with @citibeth's sentiment of:

If you don't like it, then don't use it.

@adamjstewart
Copy link
Copy Markdown
Member Author

Ping @tgamblin

@adamjstewart
Copy link
Copy Markdown
Member Author

Ping ping @tgamblin

@adamjstewart
Copy link
Copy Markdown
Member Author

Ping ping ping @tgamblin

@adamjstewart
Copy link
Copy Markdown
Member Author

Ping @tgamblin. This PR fixes #1555.

@tgamblin
Copy link
Copy Markdown
Member

@davydden @citibeth @adamjstewart Sorry this took me a while to get to.

I think this is the right thing to do. I like this PR because it:

  1. gets rid of @system as a special case in OpenSSL (I tried to preserve what was there before but obviously didn't do a particularly good job), and
  2. Users can still do whatever they want in packages.yaml -- if they want to put a placeholder or to just delegate a package to the system, they can do so.

I think all the arguments here actually make sense; I don't see anyone arguing for @system and other tag versions to remain in spack packages as special cases. As @davydden points out, using @system is going to screw up version constraints for packages that use them, but caveat emptor.

@tgamblin tgamblin merged commit db483e7 into spack:develop Sep 20, 2016
@adamjstewart adamjstewart deleted the fixes/openssl branch September 27, 2016 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants