-
Notifications
You must be signed in to change notification settings - Fork 3
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 language version to 1.8. #216
Conversation
* Update rules_kotlin to 1.8-RC-1 and Kotlin 1.8.21 * Create a macro to execute kt_jvm_library with maven kotlin stdlib included.
4b06bb3
to
d1b5384
Compare
@riemanli ended up playing with this awhile back based on your old PR. Figured we should probably get moving on it now that Kotlin 1.5 is deprecated. I can shepherd this. |
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.
Reviewed 39 of 48 files at r1, 17 of 18 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/common/crypto/BUILD.bazel
line 109 at r3 (raw file):
":security_provider", ":signing_key_storage", "//imports/java/com/google/protobuf",
Was this missed before?
src/main/kotlin/org/wfanet/measurement/storage/forwarded/BUILD.bazel
line 14 at r3 (raw file):
"//imports/kotlin/kotlinx/coroutines:core", "//src/main/kotlin/org/wfanet/measurement/common", "//src/main/kotlin/org/wfanet/measurement/common/crypto:pem_io",
Were these two missed before?
Code quote:
"//src/main/kotlin/org/wfanet/measurement/common",
"//src/main/kotlin/org/wfanet/measurement/common/crypto:pem_io",
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/common/crypto/BUILD.bazel
line 109 at r3 (raw file):
Previously, riemanli (Rieman) wrote…
Was this missed before?
Yes, this was a dep that happened to be included transitively. If we had strict deps enforcement on, this would have been an error.
The policy is to include all of your direct dependencies and not rely on transitive deps. See https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/main/docs/code-style.md#bazel-build-and-starlark
src/main/kotlin/org/wfanet/measurement/storage/forwarded/BUILD.bazel
line 14 at r3 (raw file):
Previously, riemanli (Rieman) wrote…
Were these two missed before?
Yes. Same note re: direct deps.
c5ba47d
to
3a59e8f
Compare
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.
Reviewable status: 55 of 63 files reviewed, all discussions resolved (waiting on @SanjayVas)
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.
Reviewed 39 of 48 files at r1, 16 of 18 files at r2, 1 of 8 files at r4, 7 of 7 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
Kotlin 1.5 is deprecated.
Note that this includes a breaking change where
kt_jvm_library
no longer automatically includekotlin-stdlib
andkotlin-reflect
as deps. The wrapper macro defined in//build/rules_kotlin:defs.bzl
can be used to get the old behavior.Fixes #134
Fixes #170