-
Notifications
You must be signed in to change notification settings - Fork 160
Add support for Scala 3 #429
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
Conversation
d0976bf to
4848a22
Compare
|
The CI failed because munit 0.7.25 for Scala 3 was compiled against java 11. Most likely it was by mistake because the newest munit is compiled against Java 8 |
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.
Thanks for following up with this @pikinier20!
The CI failed because munit 0.7.25 for Scala 3 was compiled against java 11. Most likely it was by mistake because the newest munit is compiled against Java 8
Ahh are you referring to 0.7.29? If so, feel free to just replace all the references of 0.7.25 to 0.7.29 then, especially if it will fix the CI for the 8 run.
EDIT: Let me update the deps, then you should be able to just rebase.
| // to be published localled in order to test | ||
| coverageEnabled.value && scalaVersion.value == "3.1.2-RC1-bin-SNAPSHOT" | ||
| ) { | ||
| } else if (coverageEnabled.value) { |
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 think instead of just checking to see if it's enabled here we should also check that it's not an older version of dotc? If not, they might end up thinking this will work and it will set -coverage-out but not do anything.
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.
Nice everything looks 🟢 ! I think if we can change this up to be more precise, we can get this merged in.
I've already force-pushed branch with updated deps. Now there's some problem with resolving source paths. I'm not sure what exactly went wrong |
|
The issue present on CI is reproducible locally. It looks strange: the plugin works under java 11+ but doesn't work under java 8. Something "eats" part of the path: instead of: It looks like the directory in which the project is present, is missing :/ EDIT with further investigations: |
|
I think it's the problem with compiler option. I removed the plugin, set the flag manually and ran tests. The behavior is the same: on java 1.8 it produces wrong source paths. |
4848a22 to
ac56ad2
Compare
132b0aa to
8602ac5
Compare
ckipp01
left a comment
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.
Thanks a lot for sending this in @pikinier20!
Re-created the pull request to fire CI.
Continuation of #428