-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Version specific URL overrides won't propagate to newer versions #8565
Version specific URL overrides won't propagate to newer versions #8565
Conversation
lib/spack/spack/package.py
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change requests for nearest_url
lib/spack/spack/package.py
Outdated
if version_urls[v]: | ||
url = version_urls[v] | ||
return url | ||
candidates = sorted([x for x in self.version_urls() if x >= version]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
lib/spack/spack/package.py
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just say "The package-level URL should be..." and omit "The idea of ..." :).
lib/spack/spack/package.py
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/spack/spack/package.py
Outdated
if not candidates: | ||
return getattr(self.__class__, 'url') | ||
|
||
return self.version_urls()[candidates[0]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
lib/spack/spack/package.py
Outdated
URL will be returned. | ||
|
||
2. Otherwise return the URL associated with the closest higher | ||
version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for @adamjstewart's suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for a second review. Implementing @adamjstewart's suggestion I could get rid completely of the function.
ping |
Ping @tgamblin |
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. I think it's fine if we miss something here and there (and I haven't found any problems skimming packages myself). |
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 ( There are a few packages mentioned in #2737 that included extra URLs to hack things to work that could be removed now:
|
Original review is stale since a new approach has been taken (#8565 (comment))
lib/spack/spack/test/packages.py
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
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.
faf5184
to
36520f1
Compare
Thanks! |
* Add url for version(1.1.0) due to missing spack#8565 and associated bugfixes Change-Id: Ibd83e37b85dc66fe2891dd0901f3b580fbe3c8ad
fixes #2737
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.