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

[RFC] Add kotlinc option support in toolchain #303

Closed

Conversation

ThomasCJY
Copy link
Contributor

Proof of concept
Add kotlinc option support into kotlin rule's toolchain, then we can add kotlinc options


# Kotlin rule toolchain configuration
load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl",
"define_kt_toolchain")
define_kt_toolchain(
    name = "kotlin_toolchain",
    api_version = "1.3",  # "1.1", "1.2", or "1.3"
    jvm_target = "1.8", # "1.6", "1.8", "9", "10", "11", or "12",
    language_version = "1.3",  # "1.1", "1.2", or "1.3"
    extra_copts = [
        "-Xplugin=./libs/kotlin/kotlinc/lib/allopen-compiler-plugin.jar",
        "-P", "plugin:org.jetbrains.kotlin.allopen:annotation=xxx",
        "-nowarn"
    ]
)

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@ThomasCJY ThomasCJY changed the title [POC] Add kotlinc option support in toolchain [RFC] Add kotlinc option support in toolchain Mar 17, 2020
@@ -107,6 +108,10 @@ _kt_toolchain = rule(
"1.3",
],
),
"extra_copts": attr.string_list(

Choose a reason for hiding this comment

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

Should probably follow naming from java_toolchain and be kotlincopts https://docs.bazel.build/versions/master/be/java.html#java_toolchain.javacopts

Choose a reason for hiding this comment

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

For context I plan to add javacopts to kt_jvm_library later, having kotlinc and javac prefixes is prob best to avoid confusion while also being on same page with rules_java

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gonna lean towards a hard no on adding javacopts to the kt_jvm_lib. When IR becomes available, it would get dicey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just quick question: what is IR?

Choose a reason for hiding this comment

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

Gonna lean towards a hard no on adding javacopts to the kt_jvm_lib. When IR becomes available, it would get dicey.

Hold on, javacoptions are for javac call for .java sources in mixed Kotlin/Java kt_jvm_library, they have nothing to do with kotlinc options :)

just quick question: what is IR?

Intermediate Representation, form of abstract (byte)code produced by Kotlin toolchain before it is transformed to actual instructions of the target platform

Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally, I'd agree, but the kotlin java compilation is currently interleaved. I'm taking a wait and see approach.

@@ -107,6 +108,10 @@ _kt_toolchain = rule(
"1.3",
],
),
"extra_copts": attr.string_list(
doc = "The extra options for kotlinc compiler.",

Choose a reason for hiding this comment

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

Suggested change
doc = "The extra options for kotlinc compiler.",
doc = "The list of extra arguments for the Kotlin compiler. Please refer to the Kotlin compiler documentation for the extensive list of possible Kotlin compiler flags.",

@restingbull
Copy link
Collaborator

I was considering something closer to java_plugin. It makes maintaining, and assigning the plugin, a lot easier when bazel handles the paths.

Additionally, I'm not thrilled with adding it to the toolchain -- compiler plugins have a lot of capabilities, and not all of them performant. I'd lean towards applying it on a library basis, as that allows fine tuning the performance of the build graph.

Finally, I am very much against passing arbitrary flags to the compiler. Without validation, it can result in all sort of fun maintenance issues. And upgrade issues. Toolchains, and the usage of them, can very easily introduce surprise into a build.

@ThomasCJY
Copy link
Contributor Author

@restingbull I also think plugin is the ideal way to go with. The use case we are trying to solve is the kotlin open annotation usages in our codebase. Currently, without being able to pass in kotlinc options we are unable to compile any of the related targets. As you can see, in gradle, it is handling this configuration as a plugin. However, I feel this plugin should also be more generic to support other features as well and I don't have enough context on what else use cases should be taken into consideration. Any suggestions?

@artem-zinnatullin
Copy link

I also agree that compiler plugins should be supported similarly to java_plugin instead of slapping them into compiler options,

however in general we need to be able to set other compiler options like -Werror and such both on toolchain and per target, I think this proposal still covers that part.

@ThomasCJY
Copy link
Contributor Author

@restingbull @cgruber any comments? We've been applying this approach in our fork to pass in kotlinc options and it works well. I agree with @artem-zinnatullin that this PR covers the part to provide a more general approach to pass in kotlinc options and java_plugin can be considered as another use scenario.

@ThomasCJY
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

add back

update optional

fix compilation
@ThomasCJY ThomasCJY force-pushed the jchen-addToolChainSupport branch from 951f2c9 to 0675cb2 Compare April 1, 2020 22:33
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@restingbull
Copy link
Collaborator

The fact a compiler plugin is added via the compiler options enough for me that this approach is too loose. See #308 for a better plugin approach.

The usages of the controlling validation I can see as reasonable, but since the compiler options are so unbounded, I really don't agree with this change. It's far better to start restrictive and open up, than to have things completely open at the start.

With validation controlling the allowed options this is roughly ok. The compiler opts need to added to all kt_*, of course, to match the java rule paradigm. (for better or worse, it's familiar)

@ThomasCJY
Copy link
Contributor Author

Seems like we have a plugin implementation available. Close this PR and feel free to reopen it in the future if we decided to support passing general kotlinc option through toolchain.

@ThomasCJY ThomasCJY closed this Apr 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants