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

feat: support jdk 8-13 during build #61

Merged
merged 1 commit into from
Aug 22, 2019

Conversation

sfat
Copy link
Contributor

@sfat sfat commented Aug 9, 2019

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What are the relevant tickets?

#60

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2019

CLA assistant check
All committers have signed the CLA.

@sfat
Copy link
Contributor Author

sfat commented Aug 9, 2019

@lili2311 , I need some help with this one. Does it have anything to do with the fact that I am not part of Snyk org or is something that is broken on my end?

@lili2311
Copy link
Contributor

lili2311 commented Aug 9, 2019

Checking

@lili2311
Copy link
Contributor

lili2311 commented Aug 9, 2019

Just looks flaky 🤔 Re-running and let's see

@lili2311
Copy link
Contributor

lili2311 commented Aug 9, 2019

These tests on cli plugins run without any snyk token, only npm token for releasing

@lili2311
Copy link
Contributor

lili2311 commented Aug 9, 2019

Different failure now, looks legit? Please take a look

@sfat
Copy link
Contributor Author

sfat commented Aug 9, 2019

I think I found the culprit:

  38 passing (10m)

And after 10 min (i.e. 600seconds) it will timeout ("test-system": "tap -Rspec --timeout=600 ./test/system/*.test.[tj]s")

I'm guessing since the stages now will take longer since it will do multiple jdks, the VM from travis is probably slower at running the tests in parallel.

I've increased it to 700. Not sure if this will fix it.

Let me know if this is something is not desired.

@sfat
Copy link
Contributor Author

sfat commented Aug 9, 2019

Another idea would be to disable the timeout. (i.e. replacing --timeout with --no-timeout).
Do you know if the timeout was put on purpose or what would be the issue if the tests do not have this flag set? Would potentially make the test stuck forever if something goes wrong?

@sfat
Copy link
Contributor Author

sfat commented Aug 12, 2019

I went ahead and created a separate branch on my fork and re-run with different things so I don't clutter this PR, but at the end I only have one problem and that is running against jdk13. (https://travis-ci.org/sfat/snyk-sbt-plugin/builds/570305508)
Seems that sbt is tied to a scala version.
And it seems that it would be needed to upgrade sbt to a version compatible with 2.12.9, which has support for jdk 13 (see: scala/bug#11608)
By default, sbt is tied to a particular scala version, but you can override the scala version like in their documentation https://www.scala-sbt.org/1.x/docs/Howto-Scala.html (disabling autoScalaLibrary).
Do we want to go ahead and upgrade sbt scala dependencies to be compatbile with jdk13 or what would be ideal to do in this case?

@miiila
Copy link

miiila commented Aug 14, 2019

@sfat Is there a tool which would allow installation of different sbt version during particular job? Please have a look at https://github.com/snyk/snyk-gradle-plugin/blob/master/.travis.yml#L30

@sfat
Copy link
Contributor Author

sfat commented Aug 14, 2019

@miiila
I found from this tweet https://twitter.com/not_xuwei_k/status/1161384606924513281 that you can override the scala version for sbt using -Dsbt.scala.version=2.12.9.
Now the problem is some of these jobs are failing because

tderr: java.lang.NoClassDefFoundError: scala/Product$class   at sbt.Inspect$DependencyTreeMode$.<init>(Inspect.scala:13)   at sbt.Inspect$DependencyTreeMode$.<clinit>(Inspect.scala)   at sbt.Inspect$.<init>(Inspect.scala:16)

Not sure if it's a good idea to have all jobs run the same scala version.

I've also looked at changing the different sbt version, but still the latest is 1.2.8 which is bound to 2.12.7 scala version which does not have the fix for jdk13 issue as mentioned in my previous comment.

I also tried using sdkman to change the version, but it doesn't use the version from the commandline, but rather the version that is set in build.properties from each test project.
I saw that sdk list sbt shows there is a newer version, 1.3.0-RC1, which would result in the same issue as sbt 1.2.8

sbt
Copying runtime jar.
Getting org.scala-sbt sbt 1.3.0-RC1  (this may take some time)...
:: retrieving :: org.scala-sbt#boot-app
	confs: [default]
	81 artifacts copied, 0 already retrieved (37332kB/306ms)
Getting Scala 2.12.8 (for sbt)...
:: retrieving :: org.scala-sbt#boot-scala
	confs: [default]
	5 artifacts copied, 0 already retrieved (19768kB/78ms)

Which is not the version that scala fixed the jdk13 issue (2.12.9)
There is also 1.3.0-RC3, but it's still using 2.12.8. This last one is not available in sdkman.

I would say until there is a compatible sbt that comes with scala 2.12.9, we can't do much about jdk13.

The main problem is that you can set whatever version you like in build.sbt (i.e. scalaVersion). That has nothing to do with sbt. sbt comes baked in with it's own scala version for some design decision.

If I go with the sbt.scala.version route, the problem appears with sbt 0.13 projects which are not compatible with 2.12.X scala and it will give me the java.lang.NoClassDefFoundError: scala/Product$class issue.

I think having the version of sbt in each projects let's leverage testing different combination of sbt (0.13 vs 1.2.8) in the different projects.

I'm thinking if it's worth adding support for jdk13 for now until things get better supported in sbt/scala/sdkman?

Maybe if sbt will use the 2.12.9 version, that we could potentially add a new test that will validate that it's working properly, but until then it's seems overkill, IMHO, since the combination of sbt+jdk13 will be a headache to configure their projects in the first place.

What do you think? Would it be an option to ditch jdk13 for now?

@sfat
Copy link
Contributor Author

sfat commented Aug 14, 2019

regarding the current failing build, it seems it there was a hiccup from maven central repo, as it seems similar to this issue: deanwampler/programming-scala-book-code-examples#8.

@sfat
Copy link
Contributor Author

sfat commented Aug 18, 2019

Any update on how do you want me to proceed here?

@lili2311
Copy link
Contributor

lili2311 commented Aug 21, 2019

👋 Don't add java 13 for now, but leave a comment to mention that it is not supported at this moment. Sounds like what we have already is probably the easiest / best solution given what you found so let's keep the projects specifying their own scala version.

So the test matrix is only Node vs Java versions what do you think?

@sfat
Copy link
Contributor Author

sfat commented Aug 21, 2019

@lili2311 , got it.
Is README.md good place to mention the jdk13 not supported yet?
The current matrix looks okay or you want me to change something?

@sfat
Copy link
Contributor Author

sfat commented Aug 21, 2019

How does this sound as comment?


Compability

snyk-sbt-plugin has been tested against jdks 8-12.

jdk13 is not currently supported (See this comment for more details)

@lili2311
Copy link
Contributor

Readme is great place for it :)

@lili2311
Copy link
Contributor

The matrix looks good to me 👍

@sfat sfat force-pushed the feat/support-jdk10-11-12-13 branch from 9390595 to d691032 Compare August 21, 2019 15:23
@lili2311 lili2311 self-assigned this Aug 22, 2019
@lili2311
Copy link
Contributor

🎉 🎉

@lili2311 lili2311 merged commit f5c53d0 into snyk:master Aug 22, 2019
@snyksec
Copy link

snyksec commented Aug 22, 2019

🎉 This PR is included in version 2.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sfat sfat deleted the feat/support-jdk10-11-12-13 branch August 22, 2019 17:16
@lili2311 lili2311 mentioned this pull request Sep 2, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants