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

Update Kotlin and Gradle #505

Merged
merged 10 commits into from
Nov 20, 2021
Merged

Update Kotlin and Gradle #505

merged 10 commits into from
Nov 20, 2021

Conversation

incendium
Copy link
Contributor

@incendium incendium commented Oct 2, 2021

This PR is intended to tackle #494 and #495 as a first step towards helping reduce the build time issues wrt. exercism/kotlin-test-runner#30.

A summary of the changes:

  • I restructured the build scripts a bit to both support the multi-module build used for testing all of the exercises via the reference implementations (the run-journey.sh script still remains broken, but running ./gradlew build in exercises will work now). All tests passed before I opened this PR.
  • In order to support the weird project layout, I added a settings.gradle.kts file to each template to control the Kotlin version instead of directly defining it in the build.gradle.kts file. This structure will play nicer with the Gradle KTS dialect in the multi-module project setup, and I have also manually verified that it works in standalone mode. It's not quite ideal but it works.
  • Gradle was updated to 7.3 and Kotlin was updated to 1.6.0.
  • I added a (kind of hacky) script to fix the Git symlinks so that the exercise tests pass. Some of the exercises use symlinks to the main source sets, and I could not get this to play well without manually fixing the symlinks.
  • Ran some automated migrations to clean up warnings and avoid deprecated code slated for removal in future releases.
  • Renamed test cases with | to use - instead as using | causes Windows systems to fail to compile the tests.

A future task is probably to update the JUnit tests to use v5 instead of v4, as I noticed some of the newer exercises are using JUnit 5. This should be relatively easy to do with the vintage test runner for JUnit 5.

Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

Only small questions. I can swiftly get this merged if fixed/responded/whatevs.

}

repositories {
mavenCentral()
}

dependencies {
implementation(kotlin("stdlib"))
implementation(kotlin("stdlib-jdk8"))
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want jdk11, or even jdk17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stdlib-jdk8 adds some extensions for Java 8 specific classes or features, like the new date/time API and some new collection methods, as well as features added in Java 7 (in the transitive stdlib-jdk7 artifact). There's no features (yet) that target Java 11+ or Java 17+ features so there are no additional stdlib artifacts for those (yet).

@SleeplessByte
Copy link
Member

I added a (kind of hacky) script to fix the Git symlinks so that the exercise tests pass. Some of the exercises use symlinks to the main source sets, and I could not get this to play well without manually fixing the symlinks.

Should probably remove these tho.

@incendium incendium marked this pull request as draft October 22, 2021 23:27
@incendium
Copy link
Contributor Author

Changed this to a draft for now. As discussed in Slack, planning on making another update once Gradle 7.3 and Kotlin 1.6 are released.

@SleeplessByte
Copy link
Member

Sounds good!

This is an invalid character for Windows filenames and causes Windows systems to fail the tests.
@incendium
Copy link
Contributor Author

incendium commented Nov 20, 2021

Made the following changes:

  • Gradle was updated to 7.3 and Kotlin was updated to 1.6.0.
  • Ran some automated migrations to clean up warnings and avoid deprecated code slated for removal in future releases.
  • Renamed test cases with | to use - instead as using | causes Windows systems to fail to compile the tests.
  • Verified all test cases passed with the reference implementations after the updates.

Thinking we're good to merge now.

@incendium incendium marked this pull request as ready for review November 20, 2021 16:31
@SleeplessByte
Copy link
Member

The configlet failure is unrelated to this PR:

�[31m`/home/runner/work/kotlin/kotlin/concepts` contains a directory named `booleans`, which is not a `slug` in the concepts array. Please add the concept to that array:�[0m
2021-11-20T22:36:31.8963106Z /home/runner/work/kotlin/kotlin/config.json

I am a bit wary that we don't have any CI that will determine if this change will break the track, but I think issues would be opened switfly if it does ;)

@SleeplessByte SleeplessByte added the x:size/large Large amount of work label Nov 20, 2021
@SleeplessByte SleeplessByte merged commit 1069767 into exercism:main Nov 20, 2021
@ErikSchierboom
Copy link
Member

@incendium Great! Could you maybe also add a test case to the Kotlin test runner tests: https://github.com/exercism/kotlin-test-runner/tree/main/tests? That test case should use the new exercise structure (with the latest Gradle version).

@incendium
Copy link
Contributor Author

Sure, I will take a look at doing that when I get some free time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants