Version specific URL overrides won't propagate to newer versions by alalazo · Pull Request #8565 · spack/spack

alalazo · 2018-06-25T10:00:59Z

The issue with #2737 was due to the logic with which the nearest URL of a version was computed. This commit changes that logic, so that overrides are local to the versions declaring them.

A unit test has been added to avoid regressions on this issue.

healther · 2018-06-25T10:49:30Z

lib/spack/spack/package.py

-        return url
+        candidates = sorted([x for x in self.version_urls() if x >= version])
+
+        # If we don't have candidates, return the default. Here we let


maybe catch the error and emit a custom message? Not sure if this should be done here or in package.py though. But the end results ideally would be that the error message contains the version and package the url should have been computed for

Hmm, I would be more for maybe documenting that this function might raise an AttributeError. If need be we could catch it in url_for_version. Waiting for @tgamblin and @adamjstewart opinion on that.

I would make this consistent with the existing behavior of returning None and look into making the errors more consistent in another PR. There are places where Spack checks whether it can get a url from a package by looking for None and I don't remember if raising an error here will break anything.

tgamblin

Minor change requests for nearest_url

tgamblin · 2018-06-25T15:49:23Z

lib/spack/spack/package.py

-            if version_urls[v]:
-                url = version_urls[v]
-        return url
+        candidates = sorted([x for x in self.version_urls() if x >= version])


No need for [] here.

We should probably make the self.versions dict an OrderedDict and guarantee that it's in order to begin with, so implementing versions_urls and nearest_url wouldn't require sorting at all. versions_urls should return an OrderedDict, so that its docstring is actually correct!

tgamblin · 2018-06-25T15:59:14Z

lib/spack/spack/package.py

+            3. If there's no such a version return the default URL from
+            the package.
+
+        The idea of the algorithm is that the package-level URL should be


Just say "The package-level URL should be..." and omit "The idea of ..." :).

tgamblin · 2018-06-25T16:15:40Z

lib/spack/spack/package.py

-        return url
+        candidates = sorted([x for x in self.version_urls() if x >= version])
+
+        # If we don't have candidates, return the default. Here we let


I would make this consistent with the existing behavior of returning None and look into making the errors more consistent in another PR. There are places where Spack checks whether it can get a url from a package by looking for None and I don't remember if raising an error here will break anything.

tgamblin · 2018-06-25T16:18:24Z

lib/spack/spack/package.py

+        if not candidates:
+            return getattr(self.__class__, 'url')
+
+        return self.version_urls()[candidates[0]]


if you take my suggestion and make version_urls return a properly sorted OrderedDict, this whole function just becomes:

return next((url for v, url in reversed(self.version_urls().items()) if v >= version), None)

adamjstewart · 2018-06-25T16:42:31Z

lib/spack/spack/package.py

+            URL will be returned.
+
+            2. Otherwise return the URL associated with the closest higher
+            version.


I would like to avoid this behavior. It seems like this is the reverse of the current behavior, which is just as non-intuitive.

For a package like:

class UrlOverride(Package): homepage = 'http://www.doesnotexist.org' url = 'http://www.doesnotexist.org/url_override-1.0.0.tar.gz' version('1.0.0', 'cxyzab') version('0.9.0', 'bcxyza', url='http://www.anothersite.org/uo-0.9.0.tgz') version('0.8.1', 'cxyzab')

I would expect that versions 0.8.1 and 1.0.0 would download from doesnotexist.org, while version 0.9.0 would download from anothersite.org.

If a package does change it's download site in newer releases, this is easy to support with a url_for_version function. In my mind, version-specific URL overrides are intended for a single one-off version, and that seems to be the case in the majority of packages that use them in Spack.

If a package does change it's download site in newer releases, this is easy to support with a url_for_version function.

To be fair it's easy to support also a one-off version... Usually I resort to url_for_version only when I need more flexibility for weird versioning schemes (e.g. where the version appear formatted in different ways across a single url). Anyhow, what you ask for shouldn't be difficult to implement. Waiting for confirmation on how to proceed..

I vote for @adamjstewart's suggestion

I believe this is also how git and hg arguments work. If you have a single version that downloads from git=..., the versions above and below this version aren't affected.

Ready for a second review. Implementing @adamjstewart's suggestion I could get rid completely of the function.

alalazo · 2018-07-04T07:26:48Z

ping

adamjstewart · 2018-07-06T03:09:23Z

Ping @tgamblin

alalazo · 2018-07-10T07:43:06Z

@scheibelp ?

scheibelp · 2018-07-10T19:13:30Z

I think it would have been worth stating in the PR description that this reverts intended behavior.

That being said, given that there are no tests for the original behavior, and since this doesn't appear to be mentioned in the documentation (e.g. http://spack.readthedocs.io/en/latest/packaging_guide.html#versions-and-fetching), I think it should be OK to make this change.

The other question is whether packages took advantage of the original behavior (e.g. hypre looks like it could have taken advantage but the way it is written now, it will not be affected by this change) - have you skimmed through packages to check if any would be broken by this?

I think it's fine if we miss something here and there (and I haven't found any problems skimming packages myself).

adamjstewart · 2018-07-10T21:03:02Z

have you skimmed through packages to check if any would be broken by this?

I just took a look through the 94 packages with version-specific URLs. I didn't find a single package that relied on the previous behavior. I think we are safe to merge!

I did find 1 package (py-quantities) that currently does not fetch due to the bug reported in #2737 but will be fixed by this PR.

There are a few packages mentioned in #2737 that included extra URLs to hack things to work that could be removed now:

bwa
jellyfish
py-backports-functools-lru-cache
py-scipy
star

Original review is stale since a new approach has been taken (#8565 (comment))

scheibelp · 2018-07-10T21:42:56Z

lib/spack/spack/test/packages.py

+    def test_urls_for_versions(self):
+        # Checks that a version directive without a 'url' argument
+        # specified uses the default url, or one that is specific
+        # to the closest newer version found.


@alalazo The last part of this comment appears stale:

or one that is specific to the closest newer version found

It looks like the default url is always used if there is not a version-specific url.

fixes spack#2737 The issue with spack#2737 was due to the logic with which the nearest URL of a version was computed. This commit changes that logic, so that overrides propagate only to older versions and not to newer ones. A unit test has been added to avoid regressions on this issue.

scheibelp · 2018-07-11T20:12:28Z

Thanks!

* Add url for version(1.1.0) due to missing spack#8565 and associated bugfixes Change-Id: Ibd83e37b85dc66fe2891dd0901f3b580fbe3c8ad

alalazo added bug Something isn't working versions labels Jun 25, 2018

alalazo requested review from adamjstewart and tgamblin June 25, 2018 10:00

alalazo mentioned this pull request Jun 25, 2018

py-ply: added version 3.11 #8383

Merged

healther reviewed Jun 25, 2018

View reviewed changes

tgamblin previously requested changes Jun 25, 2018

View reviewed changes

tgamblin reviewed Jun 25, 2018

View reviewed changes

adamjstewart requested changes Jun 25, 2018

View reviewed changes

healther mentioned this pull request Jun 26, 2018

Provide new version of sbt (simple build tool) #8576

Merged

adamjstewart approved these changes Jun 26, 2018

View reviewed changes

healther approved these changes Jul 4, 2018

View reviewed changes

scheibelp reviewed Jul 10, 2018

View reviewed changes

alalazo added 2 commits July 11, 2018 13:20

Version specific URLs are used only for one-off override.

36520f1

alalazo force-pushed the features/url_override_mechanics branch from faf5184 to 36520f1 Compare July 11, 2018 11:35

scheibelp merged commit e3a556c into spack:develop Jul 11, 2018

alalazo deleted the features/url_override_mechanics branch July 11, 2018 20:18

adamjstewart mentioned this pull request Jul 11, 2018

Remove unnecessary version-specific URL overrides #8693

Merged

tgamblin mentioned this pull request Jul 19, 2018

Allow packages without URLs #3161

Merged

muffgaga pushed a commit to electronicvisions/spack that referenced this pull request Sep 10, 2018

Backport py-scipy 1.1.0 from github ecff475

7efb1a1

* Add url for version(1.1.0) due to missing spack#8565 and associated bugfixes Change-Id: Ibd83e37b85dc66fe2891dd0901f3b580fbe3c8ad

Conversation

alalazo commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo commented Jul 4, 2018

Uh oh!

adamjstewart commented Jul 6, 2018

Uh oh!

alalazo commented Jul 10, 2018

Uh oh!

scheibelp commented Jul 10, 2018

Uh oh!

adamjstewart commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp commented Jul 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alalazo commented Jun 25, 2018 •

edited

Loading