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(ci): update graal release infrastructure #1966

Merged
merged 2 commits into from
Dec 27, 2022

Conversation

ckipp01
Copy link
Member

@ckipp01 ckipp01 commented Dec 27, 2022

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 native-image @argfile parse failure on Windows with the error Fatal error: Unsupported OptionOrigin 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. But let's see how all the other jobs go.

@ckipp01 ckipp01 force-pushed the fixWindows branch 3 times, most recently from 7ab27d5 to cced4a7 Compare December 27, 2022 15:11
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.
@ckipp01 ckipp01 requested review from adpi2 and tgodzik December 27, 2022 15:40
@ckipp01 ckipp01 marked this pull request as ready for review December 27, 2022 15:41
@ckipp01
Copy link
Member Author

ckipp01 commented Dec 27, 2022

Also, i just ripped out the sbt-ci* files. I typically don't like those as they differ from how people will run sbt locally. In this case especially since it was using sbt extras and not sbt. The only thing we lose is the [DOCS] thing where you only update the docs. But I don't really see us using that. If we do and want it back we can add it back in, but for now my mindset is to throw as much stuff as possible into the 🗑️ .

Copy link
Contributor

@tgodzik tgodzik left a 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"
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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 🤔

@ckipp01 ckipp01 merged commit 7608a82 into scalacenter:main Dec 27, 2022
@ckipp01 ckipp01 deleted the fixWindows branch December 27, 2022 17:50
@ckipp01 ckipp01 mentioned this pull request Dec 27, 2022
16 tasks
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.

None yet

2 participants