-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
Update for JDK 17 #292
Update for JDK 17 #292
Conversation
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.
Great PR. It looks right for me. It even does much more things than what you say, such as enabling androidx, updating kotlin plugin, updating compile and target SDK and updating compose version (which I think is 1.1.1 for the latest, now)
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.
Hey @Jawnnypoo thanks for the PR 😃 could you please check code formatting? CI Reports there is an error. You can automatically format the code using a grade task configured. Thank you in advance 😃
@Jawnnypoo looks like there is something wrong with the CI config after your update. Can you please take a look so we can ensure the CI is passing before releasing a new version? Thank you in advance 😃 |
@Jawnnypoo I think after merging this PR all our users will be forced to use at least JDK >= 11. Am I wrong? |
.github/workflows/build.yml
Outdated
@@ -85,6 +89,10 @@ jobs: | |||
uses: actions/setup-java@v1 | |||
with: | |||
java-version: 11 | |||
- name: Set up JDK 11 |
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.
Do you mean 17 here?
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.
Oops, good catch
.github/workflows/build.yml
Outdated
@@ -20,6 +20,10 @@ jobs: | |||
uses: actions/setup-java@v1 | |||
with: | |||
java-version: 11 |
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.
Can we remove all the references to previous JDK configs we don't need? I'd like to use the minimum version possible. So if we can use JDK 11, please keep JDK 11 and remove the versions we don't need 😃
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.
Yep, sounds good 👍
Yeah, that sounds right. The latest Android Studio plugin requires Java 11 if I recall correctly. |
Sorry this took so long, it should be good now, can we rerun this? |
There might be some issues relating to Maven Central and the release signing, but thats not something I can look too much into myself. I confirmed that this branch can be deployed to Jitpack correctly, see https://jitpack.io/com/github/Jawnnypoo/Shot/1b555c2d20/build.log |
If you wanted to try it out:
with:
configured for |
I tried this PR but I get an error running it in my project:
|
Was it working previously with the latest version of this library? |
Previously it was working with the last release of shot + java 11. But now my project uses java 17, the main release of shot didn't work, so I tried this branch but I got this error that I hadn't had before |
@pedrovgs I think you'll have to set up CI in a way that it doesn't need release signing unless it is actually pushing out the artifacts to Maven Central |
Removed release signing, should pass now |
After recording my screenshots again it's working fine! |
Verified that this also solves issue described under #268. Hope it will be released soon, for now I will use local build in project because it prevents us from upgrading AGP, ComposeCompiler and couple of other things that require JDK 17. But, as @robertgorterns wrote, it requires re-recording of all screenoshots so worth mentioning in changelog @pedrovgs |
also when the screenshot tests fail I don't get the proper error response, but the response I commented earlier |
core/src/main/scala/com/karumi/shot/screenshots/ScreenshotsDiffGenerator.scala
Outdated
Show resolved
Hide resolved
@pedrovgs as far as I can see, all the changes are approved and MR is kind of good to go. When could a release with this fix be expected? |
Co-authored-by: Nicholas Lythall <nic.lyth@gmail.com>
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.
I see two build errors
Files incorrectly formatted: /Users/runner/work/Shot/Shot/core/src/main/scala/com/karumi/shot/screenshots/ScreenshotsDiffGenerator.scala
Error: /Users/runner/work/Shot/Shot/core/src/main/scala/com/karumi/shot/screenshots/ScreenshotsDiffGenerator.scala:35:96: not found: value BufferedImage
Error: /Users/runner/work/Shot/Shot/core/src/main/scala/com/karumi/shot/screenshots/ScreenshotsDiffGenerator.scala:36:91: not found: value BufferedImage
@Jawnnypoo There are still Build errors with that latest commit |
@Jawnnypoo Thanks again for the work in this PR. Is there something missing to finish it? It seems close |
The update has been finally impemented here |
Hey there, this project has conflicts and does not work on projects that are targeting JDK 17. Most of this is due to the outdated usages of scrimage, so the bulk of this is updating that usage, but with some bonuses:
This PR sort of supersedes #288 FYI just since it adds the additional support for JDK 17. Fixes #287