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.

Sign up for GitHub

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

Jump to bottom

Use vscode-shellcheck/shellcheck-binaries (which gives us Apple Silicon, Windows, And Rasberry Pi support) #30

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Use vscode-shellcheck/shellcheck-binaries (which gives us Apple Silicon, Windows, And Rasberry Pi support) #30

merged 1 commit into from
Jan 4, 2024

Conversation

stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented Dec 6, 2023 •
edited
Loading

Use vscode-shellcheck/shellcheck-binaries (which gives us Apple Silicon, Windows, And Rasberry Pi support)

@stefanpenner stefanpenner changed the title apple silicon support aarch64 (apple silicon) support Dec 7, 2023
@stefanpenner stefanpenner changed the title aarch64 (apple silicon) support AARCH64 (apple silicon) support Dec 7, 2023
juanmolle
juanmolle reviewed Dec 19, 2023
View reviewed changes
Copy link

@juanmolle juanmolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and it is needed in my case too

@juanmolle
Copy link

@aignas is it ok for you?

aignas
aignas reviewed Jan 1, 2024
View reviewed changes
deps.bzl Outdated Show resolved Hide resolved
@aignas
Copy link
Owner

aignas commented Jan 1, 2024

Thinking about this a little more, we should support pulling all of the binaries from either the upstream or the vscode github repo. At the end of the day we would be pulling from GH, defaulting to the vscode repo would make sense. What is more, adding all of the available platforms would be also good.

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Jan 2, 2024 •
edited
Loading

If you are OK with it, I would gladly update this to pull all binaries (and add all the supported platforms) from the vscode-shellcheck/shellcheck-binaries releases.

Does it then make sense to pull from anything but those locations? If so, let me know what you would like and I can help make it so.

@aignas
Copy link
Owner

aignas commented Jan 2, 2024 via email

@stefanpenner
Copy link
Contributor Author

Let me work on that.

@stefanpenner stefanpenner force-pushed the apple-silicon branch 4 times, most recently from cc15fa0 to 0986ab6 Compare January 2, 2024 23:04
@stefanpenner stefanpenner changed the title AARCH64 (apple silicon) support Use vscode-shellcheck/shellcheck-binaries (which gives us Apple Silicon, Windows, And Rasberry Pi support) Jan 2, 2024
@stefanpenner
Copy link
Contributor Author

I've switched this to always use the binaries provided by: https://github.com/vscode-shellcheck/shellcheck-binaries
I've also included all available architectures, apple silicon, windows, raspberry pie, but was only able to test that apple silicon actually works, because I do not have windows or a raspberry pie around.

This could be good for another view.

stefanpenner
stefanpenner commented Jan 2, 2024
View reviewed changes
deps.bzl Outdated Show resolved Hide resolved
aignas
aignas reviewed Jan 2, 2024
View reviewed changes
Copy link
Owner

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's switch to x86_64 everywhere and this will be good for merging. Maybu adding a bazel skylib build_test to ensure that we can fetch each platform-specific binary would be nice to have, but I am happy to leave this out of the scope of this PR.

@stefanpenner stefanpenner force-pushed the apple-silicon branch 2 times, most recently from 395631f to 224f627 Compare January 2, 2024 23:46
@stefanpenner
Copy link
Contributor Author

stefanpenner commented Jan 2, 2024 •
edited
Loading

Let's switch to x86_64 everywhere and this will be good for merging.

✅ Complete

Maybu adding a bazel skylib build_test to ensure that we can fetch each platform-specific binary would be nice to have, but I am happy to leave this out of the scope of this PR.

That is a good idea, unfortunately I'm a little strapped for time or I would gladly doit.

aignas
aignas requested changes Jan 3, 2024
View reviewed changes
MODULE.bazel Outdated Show resolved Hide resolved
@aignas
Copy link
Owner

aignas commented Jan 3, 2024

Consider doing bazel fetch on the shellcheck target to check that repo definitions are correct for all platforms.

@stefanpenner
Use prebuilt shellcheck binaries from https://github.com/vscode-shell…
44c0e49 
…check/shellcheck-binaries

This adds support for the following architectures:
    * darwin_aarch64 (apple silicon)
    * linux_armv6hf (raspberry pi)
    * windows_x86_64
@stefanpenner
Copy link
Contributor Author

Consider doing bazel fetch on the shellcheck target to check that repo definitions are correct for all platforms.

Good catch, linux arm -> aarch change has been added, and now bazel fetch //... completes without issue. Any additional checks I should run?

@aignas aignas merged commit 81c743a into aignas:master Jan 4, 2024
6 of 7 checks passed
@aignas
Copy link
Owner

aignas commented Jan 4, 2024 •
edited
Loading

Merged as the examples are broken due to bzlmod by default, which will need to be fixed separately.

Thanks for the contribution!

@stefanpenner
Copy link
Contributor Author

@aignas thank you for the reviews! Could I ask when a release might be cut?

@aignas
Copy link
Owner

aignas commented Jan 4, 2024 via email

@aignas
Copy link
Owner

aignas commented Jan 10, 2024

Has been released in bazelbuild/bazel-central-registry#1303

@luispadron
Copy link
Contributor

Thank you for this PR! This was our last dependency to move over to bzlmod due to the arm64 binary being missing

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Jan 23, 2024 •
edited
Loading

@aignas thanks for the merge & release! Migrating our internal usage over to mainline now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanmolle juanmolle Awaiting requested review from juanmolle

@aignas aignas Awaiting requested review from aignas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stefanpenner @juanmolle @aignas @luispadron