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

feat: added gitlab support, simplified version sorting with ls-remote #58

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

stephanGarland
Copy link

@stephanGarland stephanGarland commented Jan 15, 2023

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. Additionally, it adds the --exit-code argument, which returns 2 if there are no filtered releases found - normally, it would return 0, 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.

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.
@jthegedus
Copy link
Collaborator

jthegedus commented Jan 18, 2023

@stephanGarland Hey mate, I appreciate the PR and your interest in helping with performance. I am a fan of using ls-remote, an under-utilised tool in the greater world of software.

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 1.7.7.2 which has a latest release on 2012-10-17. 2.18 is from 2018-06-21, so we're losing ~6 years of git version support. It would be interesting to see data on version usage, like we get with iOs/macOs etc.

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.

@stephanGarland
Copy link
Author

At least for Debian, current stable is 2.30.2-1. Homebrew is 2.39.1.

I'm fine with an if branch to drop back to installed tools, though - maybe I can add a one-time only message (touch a file, and if it exists, don't show the message?) suggesting the user upgrade?

@jthegedus
Copy link
Collaborator

jthegedus commented Jan 22, 2023

I would just log each time. Perhaps this is time to introduce the post-plugin-add callback into the template. I use it in https://github.com/jthegedus/asdf-gcloud to validate required dependencies and warn the user

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"
Copy link
Collaborator

@jthegedus jthegedus Jan 22, 2023

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/

Copy link
Author

@stephanGarland stephanGarland Jan 22, 2023

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.

MetricMike added a commit to MetricMike/asdf-awscli that referenced this pull request Apr 25, 2023
MetricMike added a commit to MetricMike/asdf-awscli that referenced this pull request Apr 25, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants