Fix Protobuf URLs by ax3l · Pull Request #5373 · spack/spack

ax3l · 2017-09-15T15:37:36Z

GitHub releases with additional .tar.gz source archive uploads to their actual tag tend to return random selections of which archive shall be taken. Related to #5356

This fixes it for protobuf with a hack to prioritize GitHub Source Archives over additional release artifacts. The interesting observation is also, that the pick from the list_url is also not deterministic.

I would propose to rewrite lib/spack/spack/util/web to use for pages, links and versions an ordered dict to get at least determinism into the version url fetching.

Before (note the wrong pick in the 3.3.0 & 3.4.0 release!):

$ spack checksum protobuf
==> Found 10 versions of protobuf:

  3.4.1     https://github.com/google/protobuf/archive/v3.4.1.tar.gz
  3.4.0rc3  https://github.com/google/protobuf/archive/v3.4.0rc3.tar.gz
  3.4.0rc2  https://github.com/google/protobuf/archive/v3.4.0rc2.tar.gz
  3.4.0rc1  https://github.com/google/protobuf/archive/v3.4.0rc1.tar.gz
  3.4.0     https://github.com/google/protobuf/releases/download/v3.4.0/protobuf-ruby-3.4.0.tar.gz
  3.3.2     https://github.com/google/protobuf/archive/v3.3.2.tar.gz
  3.3.1     https://github.com/google/protobuf/archive/v3.3.1.tar.gz
  3.3.0rc1  https://github.com/google/protobuf/archive/v3.3.0rc1.tar.gz
  3.3.0     https://github.com/google/protobuf/releases/download/v3.3.0/protobuf-ruby-3.3.0.tar.gz
  3.2.1     https://github.com/google/protobuf/archive/v3.2.1.tar.gz

Now:

$ spack checksum protobuf
==> Found 10 versions of protobuf:
  
  3.4.1     https://github.com/google/protobuf/archive/v3.4.1.tar.gz
  3.4.0rc3  https://github.com/google/protobuf/archive/v3.4.0rc3.tar.gz
  3.4.0rc2  https://github.com/google/protobuf/archive/v3.4.0rc2.tar.gz
  3.4.0rc1  https://github.com/google/protobuf/archive/v3.4.0rc1.tar.gz
  3.4.0     https://github.com/google/protobuf/archive/v3.4.0.tar.gz
  3.3.2     https://github.com/google/protobuf/archive/v3.3.2.tar.gz
  3.3.1     https://github.com/google/protobuf/archive/v3.3.1.tar.gz
  3.3.0rc1  https://github.com/google/protobuf/archive/v3.3.0rc1.tar.gz
  3.3.0     https://github.com/google/protobuf/archive/v3.3.0.tar.gz
  3.2.1     https://github.com/google/protobuf/archive/v3.2.1.tar.gz

scheibelp

Let me know if you disagree with the suggestions.

scheibelp · 2017-09-15T19:06:51Z

var/spack/repos/builtin/packages/protobuf/package.py

    patch('pkgconfig.patch', when='@:3.3.2')

+    def fetch_remote_versions(self):
+        """fix for https://github.com/LLNL/spack/issues/5356"""


IMO a description of the issue would be good here vs. a link.

all right, will do!
the idea of the link was just: in case we improve upon the situation generally, the link can be grep-ed and updated.

scheibelp · 2017-09-15T19:08:40Z

var/spack/repos/builtin/packages/protobuf/package.py

+    def fetch_remote_versions(self):
+        """fix for https://github.com/LLNL/spack/issues/5356"""
+        return dict(map(lambda u:
+                        (u, self.url_for_version(Version(u))),


This constrains the function to checksum only existing versions in this package. IMO it would be better to call:

spack.util.web.find_versions_of_archive([], list_url...)

Or at least I think it will work if archive_urls is an empty list. Then this function should be able to automatically find new releases.

thx for the idea, I tried to test that but it did not find version for me.
can you give me a more explicit example, please? I actually wanted to avoid the scraper in find_versions_of_archive since it is:

not deterministic

pics a random release if several are found for a version (which will randomly break the hash)

ah I think I managed, will push an update! :)

scheibelp · 2017-09-15T19:11:55Z

var/spack/repos/builtin/packages/protobuf/package.py

+                        (u, self.url_for_version(Version(u))),
+                        self.versions))
+
+    def url_for_version(self, version):


Is this strictly required if the fetch_remote_versions function is overridden? The definition in Package appears to do a direct replacement vs. searching for a releases/ directory.

since I do need that functionality in the member function above, and it's exactly what url_for_version would do I think it's useful to keep it in a separate call, no?

ax3l · 2017-09-18T22:15:06Z

@scheibelp what do you think of the changes? :)

scheibelp · 2017-09-18T22:29:34Z

var/spack/repos/builtin/packages/protobuf/package.py

+        """Ignore additional source artifacts uploaded with releases,
+           only keep known versions
+           fix for https://github.com/LLNL/spack/issues/5356"""
+        return dict(map(lambda u:


Although flake is OK with this, IMO it could be more-clearly formatted:

return dict(map( lambda u: (u, self.url_for_version(u)), spack.util.web.find_versions_of_archive( self.all_urls, self.list_url, self.list_depth) ))

Other than that your changes LGTM. No need to fight flake though if it's hard to format it in a matter that flake accepts. I'll get another chance to check this in a few hours. If you think the formatting is fine let me know.

no problem, will adjust to your recommended line-breaking

scheibelp · 2017-09-19T02:16:19Z

LGTM, I'll merge this when the tests pass. There seems to be a slowdown on travis at the moment, in particular with the mac builds, so I don't think this will happen today.

ax3l · 2017-09-19T14:29:39Z

@scheibelp all tests passed but code coverage fails in one (I don't know why)

scheibelp · 2017-09-19T17:44:15Z

Sorry I was looking over this again and I noticed the following conversation at #5373 (comment):

Is this strictly required if the fetch_remote_versions function is overridden? The definition in Package appears to do a direct replacement vs. searching for a releases/ directory.

since I do need that functionality in the member function above, and it's exactly what url_for_version would do I think it's useful to keep it in a separate call, no?

What I meant there was: I think your implementation of url_for_version is the same as the default implementation of Package.url_for_version, in particular because Package.url is https://github.com/google/protobuf/archive/v3.2.0.tar.gz. The substitution logic in the default Package.url_for_version should be smart enough to handle this. In other words, I think everything will work if you remove the definition of url_for_version in the Protobuf package.py

Does that make sense? Or do you feel it is clearer to still override Package.url_for_version?

scheibelp · 2017-09-20T04:54:49Z

@ax3l BTW the test coverage is not an issue for me (and I think it's confused) but I do think url_for_version can just use the default provided in Package.

Add review comments. Allows to automatically fetch new versions again and still only keeps the original source of the tag instead of chosing additional `.tar.gz` artifacts that come with the release.

not needed to overwrite

ax3l · 2017-09-25T09:35:48Z

@scheibelp you are right, I do not need to overwrite the url_for_version anymore! removed it.

scheibelp · 2017-09-25T17:48:01Z

Thanks!

ax3l added the url-parsing label Sep 15, 2017

ax3l requested a review from adamjstewart September 15, 2017 15:37

ax3l force-pushed the fix-protobufUrls branch from e6661ec to d68818c Compare September 15, 2017 15:39

ax3l added the new-version label Sep 15, 2017

scheibelp mentioned this pull request Sep 15, 2017

URL fetching with GitHub Releases #5356

Closed

scheibelp requested changes Sep 15, 2017

View reviewed changes

ax3l added build-error fetching and removed build-error labels Sep 17, 2017

ax3l force-pushed the fix-protobufUrls branch from d68818c to d24e7ad Compare September 17, 2017 14:45

scheibelp reviewed Sep 18, 2017

View reviewed changes

ax3l force-pushed the fix-protobufUrls branch from d24e7ad to 1ea170a Compare September 18, 2017 22:40

scheibelp approved these changes Sep 19, 2017

View reviewed changes

ax3l added 3 commits September 25, 2017 11:32

Protobuf: Forward Version Fetching

5a7cf2e

Add review comments. Allows to automatically fetch new versions again and still only keeps the original source of the tag instead of chosing additional `.tar.gz` artifacts that come with the release.

Protobuf: take default url_for_version

2627708

not needed to overwrite

ax3l force-pushed the fix-protobufUrls branch from 1ea170a to 2627708 Compare September 25, 2017 09:35

ax3l mentioned this pull request Sep 25, 2017

spack package ComputationalRadiationPhysics/graybat#144

Open

scheibelp merged commit 28dd6b3 into spack:develop Sep 25, 2017

ax3l deleted the fix-protobufUrls branch September 25, 2017 21:51

scheibelp mentioned this pull request Sep 28, 2017

Packages can require patches on dependencies #5476

Merged

9 tasks

adamjstewart mentioned this pull request May 26, 2022

protobuf: fix spack versions #30879

Merged

Conversation

ax3l commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scheibelp 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!

ax3l Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ax3l commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp commented Sep 19, 2017

Uh oh!

ax3l commented Sep 19, 2017

Uh oh!

scheibelp commented Sep 19, 2017

Uh oh!

scheibelp commented Sep 20, 2017

Uh oh!

ax3l commented Sep 25, 2017

Uh oh!

scheibelp commented Sep 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ax3l commented Sep 15, 2017 •

edited

Loading

ax3l Sep 17, 2017 •

edited

Loading