-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: added gitlab support, simplified version sorting with ls-remote #58
base: main
Are you sure you want to change the base?
Conversation
b8e1fcb
to
dd79e04
Compare
This allows for easier access to both GitHub and GitLab releases, without having to modify anything new. It also simplifies sorting by using git ls-remote's built-in --sort functionality. Since not all projects strictly follow semver, attempts were made to normalize it, such as if there is no `-` between a patch version and `rc`, `prerelease`, etc. NOTE: this does require git >= 2.18.0, so that is checked for. It also removes the -C - parameter from curl, which was attempting to resume downloads, querying the server for the byte range to do so. This is not universally supported, and if the server doesn't support it, the download will fail. Finally, where possible, it uses shell built-ins like parameter substitution over calling external commands. If this isn't possible, it minimizes the number of spawned subshells by combining commands rather than piping. This speeds up the asdf ecosystem as a whole, by minimizing syscalls.
dd79e04
to
b36b7a6
Compare
…ver, in that it must start with a digit
@stephanGarland Hey mate, I appreciate the PR and your interest in helping with performance. I am a fan of using From my prior research (asdf-vm/asdf#511 (comment) not sure how much I trust it) our current min version for git in asdf core is Ultimately, I think it would mostly be fine. In the case a user doesn't have that version, it would be good to have a sane fallback for sorting instead of just exiting. |
I would just log each time. Perhaps this is time to introduce the If not that, I would just log every time. |
url="$GH_REPO/archive/v${version}.tar.gz" | ||
|
||
if [ $IS_GITHUB -eq 0 ]; then | ||
url="$REPO/archive/v${version}.tar.gz" |
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 this is GitHub it should be $REPO/releases/
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've found an interesting difference in these endpoints, now that I look at it:
# using ccache/ccache as example repo
❯ curl -LI -w "code: %{response_code}\ncontent-type: %{content_type}\neffective_url: %{url_effective}\ntime_total: %{time_total}\n" https://github.com/ccache/ccache/archive/v4.7.4.tar.gz
...
code: 200
content-type: application/x-gzip
effective_url: https://codeload.github.com/ccache/ccache/tar.gz/refs/tags/v4.7.3
time_total: 0.158385
❯ curl -LI -w "code: %{response_code}\ncontent-type: %{content_type}\neffective_url: %{url_effective}\ntime_total: %{time_total}\n" https://github.com/ccache/ccache/releases/download/v4.7.4/ccache-4.7.4.tar.gz
...
code: 200
content-type: application/octet-stream
effective_url: https://objects.githubusercontent.com/github-production-release-asset-2e65be/461102/81edd037-87b4-4b75-ad06-9325550c3815?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20230122%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20230122T152917Z&X-Amz-Expires=300&X-Amz-Signature=fe99692a02cfdca69367c08a6205aec3a74b2736d45e4cc55bd7bd450547c9ae&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=461102&response-content-disposition=attachment%3B%20filename%3Dccache-4.7.4.tar.gz&response-content-type=application%2Foctet-stream
time_total: 0.366820
The former appears to be a direct access link, and the latter is going to S3 (and has Varnish in front of it, as it also has a via: 1.1 Varnish
response header; it was also a cache miss). Both include a redirect.
Both include x-content-type-options: nosniff
and content-disposition: attachment; filename=ccache-4.7.4.tar.gz
, but the former is explicitly declaring the type to be gzip
, and the latter directs the browser to figure it out.
Either way, I don't care and can adjust if needed, but wanted to clarify that both are valid.
* bundled installers for all, source installers for v2 * performance improvements (stolen from asdf-vm/asdf-plugin-template#58) * update readme,contributing,funding * workflows: move schedules from daily to weekly, just build on earliest/latest python supported
This allows for easier access to both GitHub and GitLab releases, without having to modify anything new.
It also simplifies sorting by using git ls-remote's built-in
--sort
functionality. Since not all projects strictly follow semver, attempts were made to normalize it, such as if there is no-
between a patch version andrc
,prerelease
, etc. Additionally, it adds the--exit-code
argument, which returns2
if there are no filtered releases found - normally, it would return0
, indicating that it had successfully connected to the remote.NOTE: this does require git >= 2.18.0, so that is checked for.
It also removes the
-C -
argument from the curl call, which was attempting to resume downloads, querying the server for the byte range to do so. This is not universally supported, and if the server doesn't support it, the download will fail.Finally, where possible, it uses shell built-ins like parameter substitution over calling external commands. If this isn't possible, it minimizes the number of spawned subshells by combining commands rather than piping. This speeds up the asdf ecosystem as a whole, by minimizing syscalls.