-
Notifications
You must be signed in to change notification settings - Fork 205
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(ci): update graal release infrastructure #1966
Conversation
7ab27d5
to
cced4a7
Compare
This was a bit of a rabbit hole, but I just went with it. With the changes to sbt-ci-release, _somehow_ the graal native image generation was failing on windows. In order to combat that this pr makes some changes to CI and to the args we're using with Graal. I'll outline the changes below. - Migrate to `graalvm/setup-graalvm` for the graal jobs. This is necessary if we want to use the newer graal versions. When I tried with `setup-scala` I got jabba erros that what I was looking for doesn't exist. Plus `setup-scala` isn't _really_ worked on anymore, so migrating away is preferable. Plus, when using `setup-graalvm` it's easy to get things like the `native-image` command on all platforms. - Bump to 22.3.0. I tried to just bump slightly, but there is a bug in 22.1.0 on windows that I hit on oracle/graal#4502. It's fixed in the newer ones, so I just bumped up. - Use `actions/setup-java` instead of `setup-scala` for jvm tests. Same reason as above with maintenance, but also built-in sbt cache. For now we're still doing a lot on 11 (where the graal stuff is on 17) but I'm trying to not change this too much for now until I get everything green and releasing. Then I'll address that. - Move `publish-binaries-windows` into `publish-binaries`. Now that we are using `setup-graalvm` it's trivial to just keep these together, using `sbt` and just call it a day. No need for two separate ones. - Remove the windows-specific `graalVMNativeImageCommand`. Again, now that we're using `setup-graalvm` we easily have the `native-image` command on the PATH so we don't need to worry about all this extra stuff. - Change a few flags: - `--no-server` wasn't valid it seemed as we were getting a message on every run that it wasn't recognized and ignored. - `-H:EnableUrlProtocols` no just uses `--enable-url-protocols`. Apparently it's frowned upon to use the `-H` stuff since it's internal, so this was just a bit of cleaning up. We do have a couple others, but there doesn't seem to be alternatives so I left them as is. - `--allow-incomlete-classpath` This is the default now, so no need to have it. You can see a green build for all 3 os's [here in my fork](https://github.com/ckipp01/bloop/actions/runs/3787821939/jobs/6439973910). But let's see how all the other jobs go.
Also, i just ripped out the |
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.
Wow! This great! Thanks for dealing with that. I didn't even know there was a separate grall action.
@@ -114,17 +135,19 @@ jobs: | |||
java -version | |||
[[ $HYDRA_LICENSE == floating-key=* ]] && mkdir -p $HOME/.triplequote && echo "$HYDRA_LICENSE" > "$HOME/.triplequote/hydra.license" || echo "Hydra license file was not created" |
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.
A random comment, I think we are no longer running any hydra tests and potentially we could remove it. We should probably talk with the triple quote people.
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.
Huh, yea I just looked. Honestly I don't know hardly anything about Hydra. However seeing that it's a paid project I have no issues ripping all the hydra stuff out. Is it something that is needed? Something that people have requested? Who uses hydra? I'll create a follow-up issue on this.
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.
Honestly I have no idea if there are people that both use hydra and Bloop or how much it was requested 🤔
This was a bit of a rabbit hole, but I just went with it. With the changes to sbt-ci-release, somehow the graal native image generation was failing on windows. In order to combat that this pr makes some changes to CI and to the args we're using with Graal. I'll outline the changes below.
graalvm/setup-graalvm
for the graal jobs. This is necessary if we want to use the newer graal versions. When I tried withsetup-scala
I got jabba erros that what I was looking for doesn't exist. Plussetup-scala
isn't really worked on anymore, so migrating away is preferable. Plus, when usingsetup-graalvm
it's easy to get things like thenative-image
command on all platforms.@argfile
parse failure on Windows with the errorFatal error: Unsupported OptionOrigin
oracle/graal#4502. It's fixed in the newer ones, so I just bumped up.actions/setup-java
instead ofsetup-scala
for jvm tests. Same reason as above with maintenance, but also built-in sbt cache. For now we're still doing a lot on 11 (where the graal stuff is on 17) but I'm trying to not change this too much for now until I get everything green and releasing. Then I'll address that.publish-binaries-windows
intopublish-binaries
. Now that we are usingsetup-graalvm
it's trivial to just keep these together, usingsbt
and just call it a day. No need for two separate ones.graalVMNativeImageCommand
. Again, now that we're usingsetup-graalvm
we easily have thenative-image
command on the PATH so we don't need to worry about all this extra stuff.--no-server
wasn't valid it seemed as we were getting a message on every run that it wasn't recognized and ignored.-H:EnableUrlProtocols
no just uses--enable-url-protocols
. Apparently it's frowned upon to use the-H
stuff since it's internal, so this was just a bit of cleaning up. We do have a couple others, but there doesn't seem to be alternatives so I left them as is.--allow-incomlete-classpath
This is the default now, so no need to have it.You can see a green build for all 3 os's here in my fork. But let's see how all the other jobs go.