Skip to content
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

custom download strategy no longer works after commit fbf474a3fd107b636eb397b0aa247f7e03f2dc72 #15169

Closed
3 tasks done
joern1811 opened this issue Apr 7, 2023 · 21 comments · Fixed by #16079
Closed
3 tasks done
Labels
bug Reproducible Homebrew/brew bug outdated PR was locked due to age stale No recent activity

Comments

@joern1811
Copy link

brew doctor output

Your system is ready to brew.

Verification

  • My "brew doctor output" above says Your system is ready to brew. and am still able to reproduce my issue.
  • I ran brew update twice and am still able to reproduce my issue.
  • This issue's title and/or description do not reference a single formula e.g. brew install wget. If they do, open an issue at https://github.com/Homebrew/homebrew-core/issues/new/choose instead.

brew config output

HOMEBREW_VERSION: 4.0.11-95-gd15f571
ORIGIN: https://github.com/Homebrew/brew
HEAD: d15f571eb6a994e7ca689721a54c5c6bff4219a0
Last commit: 28 hours ago
Core tap origin: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 29bc842d5cdffde2917bce61a1d4eb742cf6d987
Core tap last commit: 7 hours ago
Core tap branch: master
Core tap JSON: 06 Apr 17:27 UTC
HOMEBREW_PREFIX: /usr/local
HOMEBREW_CASK_OPTS: []
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_MAKE_JOBS: 16
HOMEBREW_NO_INSTALL_FROM_API: set
Homebrew Ruby: 2.6.10 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: 16-core 64-bit kabylake
Clang: 14.0.3 build 1403
Git: 2.40.0 => /usr/local/bin/git
Curl: 7.87.0 => /usr/bin/curl
macOS: 13.3-x86_64
CLT: 14.3.0.0.1.1679647830
Xcode: 14.3

What were you trying to do (and why)?

I try to "brew install" a private repo, with a custom download strategy configured.

The download strategy is taken from https://dev.to/jhot/homebrew-and-private-github-repositories-1dfh.

The strategy used is GitHubPrivateRepositoryReleaseDownloadStrategy.

If I roll back the commit, referenced by fbf474a, it works.

What happened (include all command output)?

❯ brew reinstall handelsblattgroup/tap/kundetools                                                                                                                                                                                                          
==> Fetching handelsblattgroup/tap/kundetools
==> Downloading https://github.com/handelsblattgroup/kunde2go-tools/releases/download/v3.0.0/kunde2go-tools_Darwin_x86_64.tar.gz
Error: Failed to download resource "kundetools"
Failure while executing; `/usr/bin/env /usr/local/Homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/4.0.11-95-gd15f571\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 13.3\)\ curl/7.87.0 --header Accept-Language:\ en --retry 3 --fail --location --silent --head --request GET https://github.com/handelsblattgroup/kunde2go-tools/releases/download/v3.0.0/kunde2go-tools_Darwin_x86_64.tar.gz` exited with 22. Here's the output:
curl: (22) The requested URL returned error: 404
HTTP/2 404
server: GitHub.com
date: Fri, 07 Apr 2023 14:17:13 GMT
content-type: text/plain; charset=utf-8
vary: X-PJAX, X-PJAX-Container, Turbo-Visit, Turbo-Frame, Accept-Encoding, Accept, X-Requested-With
cache-control: no-cache
strict-transport-security: max-age=31536000; includeSubdomains; preload
x-frame-options: deny
x-content-type-options: nosniff
x-xss-protection: 0
referrer-policy: origin-when-cross-origin, strict-origin-when-cross-origin
content-security-policy: default-src 'none'; base-uri 'self'; connect-src 'self'; form-action 'self'; img-src 'self' data:; script-src 'self'; style-src 'unsafe-inline'
content-length: 9
x-github-request-id: 9484:FD42:57B0878:591A642:643025E8

What did you expect to happen?

❯ brew reinstall handelsblattgroup/tap/kundetools
==> Fetching handelsblattgroup/tap/kundetools
==> Downloading https://github.com/handelsblattgroup/kunde2go-tools/releases/download/v3.0.0/kunde2go-tools_Darwin_x86_64.tar.gz
Already downloaded: /Users/xxx/Library/Caches/Homebrew/downloads/7e981c9e344d9fd5524ce1dc953191b2beceb8efba5c8283a53c1a0e4fbc4856--kunde2go-tools_Darwin_x86_64.tar.gz
==> Reinstalling handelsblattgroup/tap/kundetools
==> Caveats
zsh completions have been installed to:
/usr/local/share/zsh/site-functions
==> Summary
🍺 /usr/local/Cellar/kundetools/3.0.0: 6 files, 13.6MB, built in 4 seconds
==> Running brew cleanup kundetools...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see man brew).`

Step-by-step reproduction instructions (by running brew commands)

brew up
brew reinstall handelsblattgroup/tap/kundetools
@joern1811 joern1811 added the bug Reproducible Homebrew/brew bug label Apr 7, 2023
@jrouly
Copy link

jrouly commented Apr 7, 2023

Without a built in GitHub release asset download strategy, it's very likely that this will continue to break. There are already a number of custom GitHub download strategies, I wonder if this is something that would make sense to check in upstream (ie., in homebrew)?

@MikeMcQuaid
Copy link
Member

@jrouly We don't support installing from private repositories and wouldn't use it ourselves so: no, we're not good candidates for maintainers for this code.

@reitermarkus
Copy link
Member

reitermarkus commented Apr 7, 2023

I think I have already said this a few years ago when this broke in a similar way:

We need to deprecate this way of allowing custom download strategies and replace it with a simpler API with less surface area.

It's way too brittle in the current state since only the fetch method should be overriden but the fetch in CurlDownloadStrategy is way too complex, and technically you also need to override resolved_basename in the case of GitHub releases so it doesn't 404 on private repos.

@Brunope
Copy link

Brunope commented Apr 7, 2023

I'm having this same issue. I am also using the private release download strategy very similar to the one linked above. I was able to fix this by adding this override to my GitHubPrivateRepositoryReleaseDownloadStrategy

  def resolve_url_basename_time_file_size(url, timeout: nil)
    [download_url, "", Time.now, 0, false]
  end

Any advice on how to fix this on my end in a more robust way?

@reitermarkus
Copy link
Member

I think adding

def resolved_basename
  @filename
end

instead should work.

@Brunope
Copy link

Brunope commented Apr 10, 2023

I think adding

def resolved_basename
  @filename
end

instead should work.

Thank you for the reply. Unfortunately this did not work for me. Looks like fetch calls resolve_url_basename_time_file_size directly which 404s on this line

parsed_output = curl_head(url.to_s, timeout: timeout)

@drn
Copy link

drn commented Apr 10, 2023

We ran into this as well. The change suggested by @Brunope resolved the issue for us as well.

It sounds like private repository support won't come in homebrew core. @MikeMcQuaid do you have any guidance on how the community better manages this in the future in order to avoid these codebase conflicts? Homebrew is a great package manager for opensource packages, which is why other organizations (perhaps startups like mine) rely on an existing and known tool for distributing internal packages. The way it sounds like we're all accomplishing this is adding per-tap custom download strategies that each org needs to update every time there is a download strategy base class change.

@benweint
Copy link

Is it accurate to say that the stance of the Homebrew project is:

  1. Homebrew does not intend to ship and maintain a built-in download strategy that enables downloads from private repos; and
  2. If folks do want to write their own custom download strategies to enable this, they should inherit directly from AbstractFileDownloadStrategy, rather than CurlDownloadStrategy (since CurlDownloadStrategy doesn't offer an API-stable mechanism for modifying all outbound HTTP requests)?

@thomasnemer
Copy link

I stumbled upon the same issue, and Inheriting from AbstractFileDownloadStrategy fixed my problem.
Finding this earlier would've saved me quite some time :D

@carlocab
Copy link
Member

I stumbled upon the same issue, and Inheriting from AbstractFileDownloadStrategy fixed my problem.

Note that AbstractFileDownloadStrategy is a private API, so we really should find a solution that doesn't require inheriting from it.

Either that, or make the API public and then add unit tests to make sure we don't break it again.

@Bo98
Copy link
Member

Bo98 commented Apr 11, 2023

So the breaking change here is that the HEAD operation now fails if not found rather than ignore and continue?

@mike-carey
Copy link

mike-carey commented Apr 11, 2023

We were able to workaround this by overriding the resolve_url_basename_time_file_size in our strategy:

  def resolve_url_basename_time_file_size(url, timeout: nil)
    url = download_url
    super
  end

We specifically are using this gist for our private strategy.

@dtseiler
Copy link

Seeing similar recent breakage with our internal Artifactory downloads

@reitermarkus
Copy link
Member

reitermarkus commented Apr 11, 2023

Okay, so for private GitHub releases, it should be possible to do this without any custom download strategy by specifying the headers in the formula and using the API URL directly, e.g.

  url "https://api.github.com/repos/Homebrew/homebrew-portable-ruby/releases/assets/103219629",
      headers: [
        "Accept: application/octet-stream",
        "Authorization: bearer #{ENV["HOMEBREW_GITHUB_API_TOKEN"]}"
      ]

Of course you'll likely have to specify version manually in this case. Alternatively, you'll need to complain to GitHub and demand they support private release downloads with the standard URL and a Authorization header to make this easier.

@danieljamesscott
Copy link

Of course you'll likely have to specify version manually in this case. Alternatively, you'll need to complain to GitHub and demand they support private release downloads with the standard URL and a Authorization header to make this easier.

Sorry, I don't understand this. What do you mean by "standard URL"? https://github.com/ORG_NAME/REPO_NAME/archive/refs/tags/v1.tar.gz works, after following the redirect.

@reitermarkus
Copy link
Member

https://github.com/ORG_NAME/REPO_NAME/archive/refs/tags/v1.tar.gz works, after following the redirect.

So why does the custom download strategy need to fetch the asset ID in the first place if the URL works as is?

@danieljamesscott
Copy link

https://github.com/ORG_NAME/REPO_NAME/archive/refs/tags/v1.tar.gz works, after following the redirect.

So why does the custom download strategy need to fetch the asset ID in the first place if the URL works as is?

It doesn't! 😁 (At least the custom strategy I'm using doesn't do that, it uses the url directly). I'll give your solution a try. Thanks!

@reitermarkus
Copy link
Member

The one linked to in the issue does this, I guess it's not needed anymore. The Accept: application/octet-stream header is also not needed then I assume.

@reitermarkus
Copy link
Member

Your URL is a repository archive (tarball) URL though, not a release asset URL, so there may still be a difference.

@danieljamesscott
Copy link

Your URL is a repository archive (tarball) URL though, not a release asset URL, so there may still be a difference.

Ahh yes, but I can confirm that your code above does work, when using the archive URL (Which is a link to the asset from the release page). I no longer need a custom strategy, thanks!

ifutivic added a commit to superbet-group/homebrew-betting.platform that referenced this issue Apr 13, 2023
Brew changed how CurlDownloadStrategy works in a recent commit which broke our custom download strategy.

Ref. Homebrew/brew#15169
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label May 16, 2023
@MikeMcQuaid MikeMcQuaid closed this as not planned Won't fix, can't repro, duplicate, stale May 16, 2023
flood4life added a commit to meetcleo/homebrew-cleo that referenced this issue Jun 6, 2023
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Reproducible Homebrew/brew bug outdated PR was locked due to age stale No recent activity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

14 participants