Fix SVN downloads of versioned formula by munging directory names #4358

apjanke · 2018-06-20T00:21:02Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew tests with your changes locally?

The SubversionDownloadStrategy is broken for versioned formulae, because it includes an "@" in the resulting directory names, and "@" is a special character to Subversion. This PR fixes it by munging the cache and staging directory names to use "_AT_" instead of "@", avoiding the special character.

There's currently no test for the SubversionDownloadStrategy, and it doesn't look like download strategies are tested in the context of a stage operation. Suggestions welcome as to how to do a decent test on this.

Details

To reproduce, create a versioned formula that uses an SVN download as its main download, for example, netpbm. Do a brew install -s on it, and it'll error during the fetch step.

$ brew install -s octave-app/octave-app/[email protected]                                                                                                                                                      master
==> Installing [email protected] from octave-app/octave-app
==> Cloning svn://svn.code.sf.net/p/netpbm/code/stable
Updating /Users/janke/Library/Caches/Homebrew/[email protected]
==> Checking out 3244
svn: E200009: '/Users/janke/Library/Caches/Homebrew/[email protected]': a peg revision is not allowed here
Error: Failed to download resource "[email protected]"
Failure while executing: svn up -q /Users/janke/Library/Caches/Homebrew/[email protected] -r 3244

References

MikeMcQuaid

Nice work!

MikeMcQuaid · 2018-06-20T07:38:36Z

Library/Homebrew/download_strategy.rb

@@ -665,6 +665,11 @@ def initialize(name, resource)
    @url = @url.sub("svn+http://", "")
  end

+  def cache_filename
+    # "@" is a special character for Subversion. Keep it out of the cache dir name.
+    "#{name.gsub("@", "_AT_")}--#{cache_tag}"


Do you think we need the _? I also wonder about using - instead.

Switched to -. I like having some separator there for readability.

MikeMcQuaid · 2018-06-20T07:39:16Z

Library/Homebrew/resource.rb

@@ -67,7 +67,8 @@ def escaped_name
  end

  def download_name
-    name.nil? ? owner.name : "#{owner.name}--#{escaped_name}"
+    # Must not have "@" in download_name because it's a special character to Subversion
+    (name.nil? ? owner.name : "#{owner.name}--#{escaped_name}").gsub("@", "_AT_")


if name would be a bit nicer (I know it's not your code). Would be good to reuse the gsub; perhaps a shared resource function?

Refactored it to use an if name.

apjanke · 2018-06-23T04:16:49Z

Ping! Any outstanding changes still needed on this? Would be nice to get this merged, since I'm actively encountering this error in a project.

MikeMcQuaid

Sorry I had an unsubmitted review and am on vacation with spotty internet.

MikeMcQuaid · 2018-06-21T12:56:37Z

Library/Homebrew/download_strategy.rb

@@ -665,6 +665,11 @@ def initialize(name, resource)
    @url = @url.sub("svn+http://", "")
  end

+  def cache_filename
+    # "@" is a special character for Subversion. Keep it out of the cache dir name.
+    "#{name.gsub("@", "-AT-")}--#{cache_tag}"


Can the .gsub("@", "-AT-") go into a function somewhere rather than duplicating below? Note that below the duplication is not correct too 😉

apjanke · 2018-06-24T06:28:39Z

No worries. Enjoy your vacation; this stuff can wait!

When you have time, I've pushed an amendment with a refactor that addresses your latest comments, consolidating the gsub into a single shared method.

MikeMcQuaid

Brew style is failing

MikeMcQuaid · 2018-06-25T06:38:18Z

Library/Homebrew/download_strategy.rb

@@ -665,6 +665,11 @@ def initialize(name, resource)
    @url = @url.sub("svn+http://", "")
  end

+  def cache_filename
+    # "@" is a special character for Subversion. Keep it out of the cache dir name.


Feels like this comment should be for safe download name

Moved comment to safe_download_name.

MikeMcQuaid · 2018-06-25T06:40:35Z

Library/Homebrew/resource.rb

+    Resource.safe_download_name(
+      if name
+        "#{owner.name}--#{escaped_name}"
+      else


Feels weird having an if inside a function call. I’d assign a new variable outside it.

Rewrote it to use a temporary variable. Moved & merged comment to safe_download_name.

apjanke · 2018-06-25T06:45:09Z

Amended to address comments; brew style fixed.

MikeMcQuaid · 2018-06-25T07:19:28Z

Library/Homebrew/formula_installer.rb

@@ -727,6 +727,7 @@ def build

    formula.update_head_version

+    puts "Testing formula prefix for emptiness: formula.prefix='#{formula.prefix}'"


Oops. Fixed.

Sorry for all the hassle here.

MikeMcQuaid · 2018-06-25T07:58:53Z

Thanks @apjanke!

apjanke · 2018-06-25T08:05:58Z

Yay, thank you! And you're welcome!

Homebrew/brew#4358 fixes downloads of versioned formulae from SVN repos, and is in master Homebrew now. No need for our special patch branch any more.

ilovezfs · 2018-06-25T13:54:31Z

This seems to have changed the URLs that brew mirror uses for @ formulae.

Before revert:

iMac-TMP:Homebrew joe$ brew mirror imagemagick@6
==> Downloading https://dl.bintray.com/homebrew/mirror/imagemagick%406-6.9.10-3.tar.xz
Already downloaded: /Users/joe/Library/Caches/Homebrew/imagemagick-AT-6-6.9.10-3.tar.xz
==> Uploading to https://dl.bintray.com/homebrew/mirror/imagemagick-AT-6-6.9.10-3.tar.xz
^C

After revert:

iMac-TMP:Homebrew joe$ brew mirror imagemagick@6
==> Downloading https://dl.bintray.com/homebrew/mirror/imagemagick%406-6.9.10-3.tar.xz
Already downloaded: /Users/joe/Library/Caches/Homebrew/[email protected]
==> Uploading to https://dl.bintray.com/homebrew/mirror/[email protected]

CC @apjanke

apjanke · 2018-06-25T15:48:58Z

Darn it!

Do the file upload names to Bintray matter? If so, best revert this PR, and I'll have to come up with a more detailed PR that tracks both original filenames and local filenames separately.

MikeMcQuaid · 2018-06-25T17:21:18Z

I’d suggest tracking or calculating the original url filename in mirror as that’ll be the only place this matters.

ilovezfs · 2018-06-25T21:11:31Z

Do the file upload names to Bintray matter?

mirror "https://dl.bintray.com/homebrew/mirror/imagemagick-AT-6-6.9.10-3.tar.xz"

should work if the change is intentional.

Personally, I'm really unconvinced that having the @ formula names in the cache files be munged just for the sake of svn was a desirable change. Hunting for

[email protected]
gcc@7--patch-863957f90a934ee8f89707980473769cff47ca0663c3906992da6afb242fb220.patch

is a lot nicer than
gcc-AT-7-7.3.0.tar.xz
gcc-AT-7--patch-863957f90a934ee8f89707980473769cff47ca0663c3906992da6afb242fb220.patch

Also does this invalidate cleanup? Is it still going to know how to cleanup the tarballs with the old names or did they all just get stranded forever?

MikeMcQuaid · 2018-06-26T06:08:22Z

I didn’t review this sufficiently. I was under the impression from the PR description that this only affected SVN resources. I think it should be reverted and scoped just to them, unfortunately.

apjanke · 2018-06-27T10:39:36Z

Okay, sorry. I will re-submit a new PR scoped only to SVN downloads when I'm able to figure out how to properly unit test it.

MikeMcQuaid requested changes Jun 20, 2018

View reviewed changes

MikeMcQuaid requested changes Jun 23, 2018

View reviewed changes

MikeMcQuaid approved these changes Jun 25, 2018

View reviewed changes

MikeMcQuaid requested changes Jun 25, 2018

View reviewed changes

MikeMcQuaid reviewed Jun 25, 2018

View reviewed changes

Fix SVN downloads of versioned formula by munging directory names

b72c4e3

MikeMcQuaid approved these changes Jun 25, 2018

View reviewed changes

MikeMcQuaid merged commit 7506abe into Homebrew:master Jun 25, 2018

apjanke deleted the fix-subversion-download-of-versioned-formulae branch June 25, 2018 08:05

MikeMcQuaid mentioned this pull request Jun 26, 2018

Revert "Fix SVN downloads of versioned formula by munging directory names" #4376

Merged

ffeu mentioned this pull request Jul 25, 2018

Adjusting SVN commands to consider the @ symbol #4551

Merged

6 tasks

lock bot added the outdated PR was locked due to age label Jul 27, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SVN downloads of versioned formula by munging directory names #4358

Fix SVN downloads of versioned formula by munging directory names #4358

apjanke commented Jun 20, 2018 •

edited

Loading

MikeMcQuaid left a comment

MikeMcQuaid Jun 20, 2018

apjanke Jun 21, 2018

MikeMcQuaid Jun 20, 2018

apjanke Jun 21, 2018

apjanke commented Jun 23, 2018

MikeMcQuaid left a comment

MikeMcQuaid Jun 21, 2018

apjanke commented Jun 24, 2018

MikeMcQuaid left a comment

MikeMcQuaid Jun 25, 2018

apjanke Jun 25, 2018

MikeMcQuaid Jun 25, 2018

apjanke Jun 25, 2018

apjanke commented Jun 25, 2018

MikeMcQuaid Jun 25, 2018

apjanke Jun 25, 2018

MikeMcQuaid commented Jun 25, 2018

apjanke commented Jun 25, 2018

ilovezfs commented Jun 25, 2018

apjanke commented Jun 25, 2018

MikeMcQuaid commented Jun 25, 2018

ilovezfs commented Jun 25, 2018

MikeMcQuaid commented Jun 26, 2018

apjanke commented Jun 27, 2018

		@@ -727,6 +727,7 @@ def build

		formula.update_head_version

		puts "Testing formula prefix for emptiness: formula.prefix='#{formula.prefix}'"

Fix SVN downloads of versioned formula by munging directory names #4358

Fix SVN downloads of versioned formula by munging directory names #4358

Conversation

apjanke commented Jun 20, 2018 • edited Loading

Details

References

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apjanke commented Jun 23, 2018

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apjanke commented Jun 24, 2018

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apjanke commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jun 25, 2018

apjanke commented Jun 25, 2018

ilovezfs commented Jun 25, 2018

apjanke commented Jun 25, 2018

MikeMcQuaid commented Jun 25, 2018

ilovezfs commented Jun 25, 2018

MikeMcQuaid commented Jun 26, 2018

apjanke commented Jun 27, 2018

apjanke commented Jun 20, 2018 •

edited

Loading