rules_kotlin icon indicating copy to clipboard operation
rules_kotlin copied to clipboard

Passing kotlinc options

Open jmmk opened this issue 7 years ago • 23 comments

I don't see any documentation around hooks into the Kotlin compiler. Specifically, I want to be able to pass -Xcoroutines=enable

jmmk avatar Feb 14 '18 05:02 jmmk

I decided to hide the compiler switches when I forked from pubref:

  • To ensure user flags cannot break the builder --e.g., kapt arguments are managed by the kotlin builder.
  • For future proofing the design across the different aspects. We have to share the an overall rule design with native and js.
  • To allow us to have a configuration contract between the intellij plugin and the rules repo.

Ideally toolchains are the mechanism we should use for configuring the compiler. Currently we don't know what is going to happen to coroutines when it goes live, will the flag still be required / be available, will there be a core coroutines library included in the compiler distribution.

A quick fix for now might be to look for certain tags --e.g., kotlinx-coroutines and add the flag in skylark if its present.

hsyed avatar Feb 14 '18 07:02 hsyed

The java rules have javacopts. Is that different from allowing kotlincopts?

jmmk avatar Feb 14 '18 13:02 jmmk

Imo yes, Kotlinc flags should be configured by the rules / builder. I'm up for making the coroutines configurable just not by opening up the flags. If we open up arbitrary kotlinc opts it would have to have to pretty much disallow most of the flags.

Toolchains should be used for this for now the toolchain should allow setting-language-version, -api-version, -jvm-target. We could add a feature-flag attribute that takes strings which translate to experimental kotlinc configurations.

hsyed avatar Feb 14 '18 15:02 hsyed

Let's not call it "no" at this point, rather let's add support for well known flags as we get requests for them. I'm sympathetic to the fact that we do, IIUC, allow this for the java builder, but I'd want to see specific examples that we want to support.

On Wed, 14 Feb 2018 at 07:50 Hassan Syed [email protected] wrote:

Imo yes, Kotlinc flags should be configured by the rules / builder. I'm up for making the kotlinc-coroutines configurable just not by opening up the flags. If we open up arbitrary kotlinc opts it would have to have to pretty much disallow most of the flags.

Toolchains should be used for this for now the toolchain should allow setting-language-version, -api-version, -jvm-target. We could add a feature-flag attribute that takes strings which translate to experimental kotlinc configurations.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_kotlin/issues/23#issuecomment-365649722, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUN4muTgFmPt0zvPofs44vhfnDVF768ks5tUwDKgaJpZM4SEzkw .

cgruber avatar Feb 14 '18 16:02 cgruber

I have added the kotlin toolchain boilerplate for kt_jvm_. This makes some of the flags that are currently hard coded configurable.

Skydoc is broken now it doesnt recognise the toolchain attribute and hust errors out. I think we should just follow the go repo and write the rule documentation in rst.

I’m am thinking that the tags attribute should be sufficient to convey some non standard tunables coroutine can go in here as well, so when it becomes redundant we won’t have baggage.

If you agree on this approach we need a labelling scheme Coroutines is a universal flag afaik/

hsyed avatar Feb 15 '18 03:02 hsyed

Coroutines has the production ready stamp from JB. Might as well enable it with the toolchain commit.

hsyed avatar Feb 15 '18 08:02 hsyed

@jmmk @cgruber I just implemented toolchains in #24 and added the coroutine flag as a top level configurable enabled by default. The intellij plugin needs to be configured as well, so having it on the toolchain makes sense.

hsyed avatar Feb 15 '18 23:02 hsyed

@hsyed Just tried out the updated rules

Compile step (bazel build //app:run_all) went fine, but when I try to run (bazel run //app:run_all) I get:

INFO: Running command line: bazel-bin/app/run_all
Unrecognized option: -Xcoroutines
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
ERROR: Non-zero return code '1' from command: Process exited with status 1

jmmk avatar Feb 16 '18 00:02 jmmk

how weird ! will check. is this a java_binary or kt_jvm_binary` ?

hsyed avatar Feb 16 '18 00:02 hsyed

@hsyed oops, my fault - I had thrown -Xcoroutines=enable in there when I was trying to get it working previously. It runs successfully with the new rules!

jmmk avatar Feb 16 '18 00:02 jmmk

@hsyed I would like to enable the -Werror flag. Are you still thinking of having individual options added, as is the case now? If so, would you be open to a PR that added this option? Thank you for the great work on rules_kotlin!

trevorsummerssmith avatar Apr 27 '18 18:04 trevorsummerssmith

@trevorsummerssmith sure, I think this flag should also be settable via the command line -- I think in a sizeable mono repo such flags are always disabled so being able to provide it as a build flag makes sense. So making it configurable in both ways makes sense.

The flag also doesn't need to go into the toolchain_info, afaik it wouldn't make intellij behave differently. The flags currently modelled do make a change to the intellij behaviour.

hsyed avatar Apr 27 '18 20:04 hsyed

I think this needs to be completely revisited. There does need to be a way to pass in compiler options - or at least, a very clear way to pass in annotation processing options to kapt. Quite a few options are necessary for everything from dagger to auto_value_gson, etc. These tweak compiler output for optimization, but also supply values you might want to tweak per-target, like "default absent collecitons are empty not null" for auto-value-gson.

Maybe this is a different feature, since it's specifically annotation processing flags I'm concerned with, but it's not clear to me that it isn't still generalizable to the compilers. At the very least, being able to pass javacopts to the java compiler run under the hood is pretty critical. I can see a desire to prefer toolchains or another mechanism, but keeping people from passing them via flags removes the place one might be able to workaround a missing toolchain exposed feature, and thus ties people rather deeply to the release cycle of the rules (or makes them fork it)

cgruber avatar Feb 19 '19 19:02 cgruber

It would be huge to be able to specify kotlin compiler plugins as well. I know it's possible via command line options, but might also be worth looking into in an analogous way to how annotation processor plugins work.

jasonwyatt avatar Oct 24 '19 21:10 jasonwyatt

It would be huge to be able to specify kotlin compiler plugins as well. I know it's possible via command line options, but might also be worth looking into in an analogous way to how annotation processor plugins work.

+1, being able to configure Kotlin compiler plugins (all-open, no-arg...) would be huge. @jasonwyatt you mentioned command line options, is there an existing mechanism to use them in Bazel? I can't think of a way to achieve this without forking the Kotlin rules.

FrenchSam avatar Oct 25 '19 21:10 FrenchSam

I do think it would have to involve changes to the rules...

On Fri, Oct 25, 2019, 2:54 PM FrenchSam [email protected] wrote:

It would be huge to be able to specify kotlin compiler plugins as well. I know it's possible via command line options, but might also be worth looking into in an analogous way to how annotation processor plugins work.

+1, being able to configure Kotlin compiler plugins (all-open, no-arg...) would be huge. @jasonwyatt https://github.com/jasonwyatt you mentioned command line options, is there an existing mechanism to use them in Bazel? I can't think of a way to achieve this without forking the Kotlin rules.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_kotlin/issues/23?email_source=notifications&email_token=AABBCLQ7WB4PPPGXNSPETALQQNTIJA5CNFSM4EQTHEYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECJVFPA#issuecomment-546525884, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBCLRYGXPHBVNCTZCAVU3QQNTIJANCNFSM4EQTHEYA .

jasonwyatt avatar Oct 26 '19 21:10 jasonwyatt

Hi, is there an update here. Is this something that support is planned for.

notsatyarth avatar Dec 09 '19 07:12 notsatyarth

Also would love the ability to pass in a flag to suppress warnings

nkoroste avatar Feb 05 '20 23:02 nkoroste

So, yes, there are plans to support various flags, but it's not clear (yet) how we'll do that (in terms of the API). It will at least involve some facility for flags passed in the toolchain (global to the build) but also we need some per-target configuration. Whether that's similar to javac_opts or not is unclear, but it's an area we hope to address. It is, however, not an area we are likely to get to before we have a kt_plugin rule, because some of the needs of that feature will direct the way we do configurability.

Sorry I can't be more specific, but I will be triaging and prioritizing this issue later this month.

cgruber avatar Feb 06 '20 05:02 cgruber

Any updates on this? We're about to fork these rules just for this feature since it's breaking all of our unit tests - there is no way of getting @OpenForTesting annotation to work.

nkoroste avatar Mar 11 '20 20:03 nkoroste

@hsyed @cgruber I opened a PR to add kotlinc options support in kotlin toolchain, ptal and let me know what you guys think https://github.com/bazelbuild/rules_kotlin/pull/303

ThomasCJY avatar Mar 17 '20 00:03 ThomasCJY

:wave: Hello, we would also need to pass some options that aren't currently whitelisted and without which we are blocked in our migration to Bazel, so I'd like to bump this. For example we want -Werror (which is supported), but then also -Xsuppress-version-warnings (which is not).

In all honesty, I get the "we need to protect the kotlin rules from the unbound space of compiler options", but I really think that providing no way to actually bypass that for advanced usage is a bit hindering, precisely because of the vastness of the space of compiler options (some people are bound to need some of those options in some cases). Forcibly white-listing options introduces quite some friction in those cases.

I think there should be a mechanism to pass arbitrary options. Let it have a huge docs warning that "this may be dangerous for your build", but I think we should have the freedom to do it any way if we really need to.

We'll probably end up forking or patching rules_kotlin like others have done because of this, which I don't find ideal.

redsun82 avatar Mar 22 '24 07:03 redsun82

-Xsuppress-version-warnings is supported in the latest version.

Still, our very specific use case requires us to pass different -language-version for different Bazel targets (we are creating static code analyzers for Kotlin, and in order to support multiple versions we need to rebuild our code against various compiler versions with different -language-version). I don't think a language_version field should be added to kotlinc_options, because that'd be too much a foot gun for common rules_kotlin users. However I think a way to "unsafely" pass options to the underlying compiler would be most welcome.

For the moment, we are down to applying a patch to rules_kotlin.

redsun82 avatar Apr 09 '24 13:04 redsun82