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

refactor: Use Scala Native instead of native image #4738

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Jan 24, 2025

Fixes #4729

@tgodzik tgodzik force-pushed the build-scala-native branch 5 times, most recently from 006a710 to edcfb8e Compare January 24, 2025 19:15
@tgodzik tgodzik requested a review from kitbellew January 24, 2025 19:15
@tgodzik tgodzik marked this pull request as ready for review January 24, 2025 19:15
@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 24, 2025

Looks like it's simpler than I thought. I also added windows and macos runners.

@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 24, 2025

I wonder if we should be so strict with the version incompatibility? Maybe we should allow version of the image higher than the version in the config?

@kitbellew
Copy link
Collaborator

I wonder if we should be so strict with the version incompatibility? Maybe we should allow version of the image higher than the version in the config?

where does this come into play?

@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 24, 2025

I wonder if we should be so strict with the version incompatibility? Maybe we should allow version of the image higher than the version in the config?

where does this come into play?

We have a check for scala native that if the version of the binary is different than the one in config it will throw an exception.

@kitbellew
Copy link
Collaborator

I wonder if we should be so strict with the version incompatibility? Maybe we should allow version of the image higher than the version in the config?

where does this come into play?

We have a check for scala native that if the version of the binary is different than the one in config it will throw an exception.

I wouldn't do that. we must use the version specified in the config file, that's the contract. how is "higher version" better than "lower version"? both can produce a slightly different formatting, even if higher likely won't fail on newer configuration parameters.

@tgodzik tgodzik force-pushed the build-scala-native branch from edcfb8e to ceaffc4 Compare January 27, 2025 19:54
@tgodzik tgodzik force-pushed the build-scala-native branch 2 times, most recently from 8be93ac to 5ea40da Compare January 27, 2025 20:25
@tgodzik tgodzik force-pushed the build-scala-native branch from 5ea40da to d18fa6e Compare January 27, 2025 20:35

For macOS and Linux, it's possible to download pre-built GraalVm native binaries
with instant startup and fast performance for short-lived Scalafmt runs.
It's possible to download pre-built Scala Native binaries
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be controversial, but I feel like fixing the install-scalafmt-native.sh is a lot more work and scala cli already properly downloads binaries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And also on all platforms

Copy link
Collaborator

Choose a reason for hiding this comment

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

that sounds good to me, no duplication

curl https://raw.githubusercontent.com/scalameta/scalafmt/master/bin/install-scalafmt-native.sh | \
bash -s -- $VERSION $INSTALL_LOCATION
scalafmt-native --help # should show version @STABLE_VERSION@
scala-cli format
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will read the scalafmt version if it exists and download the proper binary. We will need to update it and drop the additional repository, but it should be a smooth transition.

kitbellew
kitbellew previously approved these changes Jan 27, 2025

For macOS and Linux, it's possible to download pre-built GraalVm native binaries
with instant startup and fast performance for short-lived Scalafmt runs.
It's possible to download pre-built Scala Native binaries
Copy link
Collaborator

Choose a reason for hiding this comment

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

that sounds good to me, no duplication

@tgodzik tgodzik force-pushed the build-scala-native branch from 70da3bd to b902291 Compare January 27, 2025 21:11
@tgodzik tgodzik requested a review from kitbellew January 27, 2025 21:38
uses: graalvm/setup-graalvm@v1
with:
fetch-depth: 0
- uses: actions/setup-java@v4
Copy link
Collaborator

Choose a reason for hiding this comment

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

a comment for separate consideration: if we are using java 21 for this, should we disable java 8 compatibility in native builds (if they apply, that is)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but the main difference would be that we could use some method from newer JDKs for the native code itself. Not sure it's worth it really. I've put JDK 21, but that might not matter also. Just picked newest LTS

@kitbellew kitbellew merged commit 8672705 into scalameta:main Jan 28, 2025
33 checks passed
@tgodzik tgodzik deleted the build-scala-native branch January 28, 2025 16:42
@kitbellew
Copy link
Collaborator

@tgodzik somehow didn't occur to me... should we restore native image for now and keep both until all possible kinks have been ironed out?

@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 28, 2025

We can add the script back so that people can migrate, though I would recommend using Scala CLI in that case.

For the script to work with the new setup we would have to support windows etc. Alternatively, I can make the script work with the new setup as well only for those systems that it used to work for.

@kitbellew
Copy link
Collaborator

apparently, people are saying (on discord) that scala-cli doesn't work. yet, at least. so, in order to avoid disruptions to existing setups, should we restore what we had before, in addition to the new stuff, and remove it later, when we've confirmed everything is working for everyone?

@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 28, 2025

apparently, people are saying (on discord) that scala-cli doesn't work. yet, at least. so, in order to avoid disruptions to existing setups, should we restore what we had before, in addition to the new stuff, and remove it later, when we've confirmed everything is working for everyone?

I did a fix for Scala CLI, forgot to publish the latest binaries. It's running now https://github.com/VirtusLab/scalafmt-native-image/releases/tag/v3.8.6

And added the script back, so that should also restore things for now. I can add a warning maybe for newer versions to use SCala CLI? I will make sure the releases are on time and migrate soon to the original binaries here

@kitbellew
Copy link
Collaborator

And added the script back, so that should also restore things for now. I can add a warning maybe for newer versions to use SCala CLI? I will make sure the releases are on time and migrate soon to the original binaries here

We could notate that native image will be removed in early 2025.

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.

Publish images using Scala native instead of Native Image
2 participants