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

Setup java cache #300

Merged
merged 4 commits into from
Oct 21, 2022
Merged

Setup java cache #300

merged 4 commits into from
Oct 21, 2022

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Jun 12, 2022

Relates to sbt/sbt-github-actions#104

Sbt plugins are not my area of expertise. If someone could give me some pointers on how to add test coverage that would be great. https://github.com/pjfanning/sbt-typelevel/tree/setup-java-cache/github-actions currently has no src/test directory.

@armanbilge
Copy link
Member

Hi, thanks for the PR! This is very exciting, it looks like support for sbt was added recently :)

Have you been using this feature in your projects? I ask because it seems there is still some WIP fixes for sbt so I am not sure how stable it is yet.

Although, I think our custom cache step currently has the same bug 😅

If someone could give me some pointers on how to add test coverage that would be great.

Truthfully this project is shamefully lacking in tests. See #26. Because the plugin is self-hosting I try to dog-food as much as possible and I think that is sufficient in this case.

@armanbilge
Copy link
Member

One shortcoming of this change is that caching would no longer be supported for anyone who uses a specific GraalVM version, since that doesn't use the setup-java action.

case jv @ JavaSpec(JavaSpec.Distribution.GraalVM(graalVersion), version) =>
WorkflowStep.Use(
UseRef.Public("DeLaGuardo", "setup-graalvm", "5.0"),
name = Some(s"Setup GraalVM (${jv.render})"),
cond = Some(s"matrix.java == '${jv.render}'"),
params = Map("graalvm" -> graalVersion, "java" -> s"java$version")
) :: Nil

@armanbilge
Copy link
Member

Sorry, one more question about this: is there any way to customize the cache key? This has been on the todo because we've been having problems with reliability of caching. See:

@pjfanning pjfanning marked this pull request as draft June 13, 2022 08:13
@pjfanning
Copy link
Contributor Author

@armanbilge Good point about the caching when graalvm is used. The action documentation for setup-graalvm does not mention support for a cache param. So, this PR will need to be changed to re-enable the cache action when only graalvm is used in the build.

I use the setup-java caching in some of my projects but not for sbt since I use this sbt plugin. I'll have a closer look at the setup-java implementation to see if there are more issues.

Customising the cache key may be an enhancement that could be added to setup-java. I might raise an issue in that project.

@armanbilge
Copy link
Member

Awesome. Thanks for your attention to detail, much appreciated! :)

@armanbilge armanbilge mentioned this pull request Oct 8, 2022
@armanbilge armanbilge marked this pull request as ready for review October 21, 2022 01:39
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thank you very much for this! After doing some research I feel confident with this change.

I will fix caching for GraalVM in follow-up for #301.

@armanbilge armanbilge merged commit 29eab9d into typelevel:main Oct 21, 2022
@armanbilge armanbilge linked an issue Oct 21, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Smarter caching in CI
2 participants