Generate kotlinc options from kotlin-compiler-arguments-description #1406
Conversation
agluszak
requested review from
Bencodes,
ahumesky,
cgruber,
nkoroste and
restingbull
as code owners
restingbull
left a comment
restingbull
left a comment
There was a problem hiding this comment.
Aside from a few small backward compatibility issues, the approach looks ok.
Need to be solved:
- Enumerated flags will need a set of valid values -- because you'd rather have the build fail in analysis that compiler invocation. Some invocations can live very deep in a build graph, which will cause maintenance headaches.
- Flags must live on an allow list. The
friendsflag is the best example, as it pretty much cannot be used correctly, and overlaps with existing rule functionality.
Finally, please remove extra_kotlinc_args. That will inevitably be misused in large code repositories for all the reasons that the flags are treated as attributes.
...ain/starlark/core/repositories/kotlin/capabilities_2.2.bzl.com_github_jetbrains_kotlin.bazel
Show resolved
Hide resolved
...n/starlark/core/repositories/kotlin/generated_opts_2.2.bzl.com_github_jetbrains_kotlin.bazel
Outdated
Show resolved
Hide resolved
...n/starlark/core/repositories/kotlin/generated_opts_2.2.bzl.com_github_jetbrains_kotlin.bazel
Outdated
Show resolved
Hide resolved
agluszak
force-pushed
the
flags
branch
2 times, most recently
from
20c7b23 to
7993c70
Compare
… kotlin-compiler-arguments-description artifact from JetBrains. - Auto-generated options: WriteKotlincCapabilities generates both capabilities.bzl (flag existence/stability metadata) and generated_opts.bzl (full typed Starlark options) from the compiler metadata artifact. - Simplified maintenance: Manual _KOPTS dict replaced with GENERATED_KOPTS import, with _MANUAL_KOPTS for special cases only. Changes: - Add kotlin-compiler-arguments-description dependency (version must match kotlin compiler version) - Rewrite WriteKotlincCapabilities to use the new artifact - Generate generated_opts_X.Y.bzl with attr.bool for booleans, attr.string for strings, attr.string_list for arrays - Simplify opts.kotlinc.bzl to merge generated + manual options
| value_to_flag = None, | ||
| map_value_to_flag = _map_string_list_flag("-Xadd-modules"), | ||
| ), | ||
| "x_allow_no_source_files": struct( |
There was a problem hiding this comment.
This is doesn't make sense in the context of the rule invocation, as the rules will fail this case before the flag. Exposing it is misleading to the user.
| type = attr.bool, | ||
| value_to_flag = {True: ["-Xcompile-builtins-as-part-of-stdlib"]}, | ||
| ), | ||
| "x_compile_java": struct( |
There was a problem hiding this comment.
Another option that shouldn't be exposed. Java compilation happens via the java builder, so making this visible will add to the confusion.
| type = attr.bool, | ||
| value_to_flag = {True: ["-Xdebug"]}, | ||
| ), | ||
| "x_default_script_extension": struct( |
There was a problem hiding this comment.
Do any of the script flags make sense to be visible?
I'm not clear if you can compile scripts via the rule sets. There aren't any tests, at least.
| type = attr.bool, | ||
| value_to_flag = {True: ["-Xjvm-expose-boxed"]}, | ||
| ), | ||
| "x_klib": struct( |
There was a problem hiding this comment.
Any option that needs a path is not really viable to use in a rules invocation without explicit support.
The cross platform libraries would need to be included in the action inputs, and reasoning about the path is not easy.
| type = attr.bool, | ||
| value_to_flag = {True: ["-Xno-receiver-assertions"]}, | ||
| ), | ||
| "x_no_reset_jar_timestamps": struct( |
There was a problem hiding this comment.
The rules explicitly zero out the timestamps, so this is a confusing option.
| type = attr.bool, | ||
| value_to_flag = {True: ["-Xuse-inline-scopes-numbers"]}, | ||
| ), | ||
| "x_use_javac": struct( |
There was a problem hiding this comment.
Java sources are compiled via a different pipeline, so this shouldn't be visible.
| type = attr.bool, | ||
| value_to_flag = {True: ["-Xuse-javac"]}, | ||
| ), | ||
| "x_use_k2_kapt": struct( |
There was a problem hiding this comment.
kapt is explicitly supported via the rules, so this flag should not be visible.
| map_value_to_flag = _map_string_flag("-jvm-default"), | ||
| ), | ||
| "jvm_target": struct( | ||
| flag = "-jvm-target", |
There was a problem hiding this comment.
This is set by the task, so it shouldn't be visible.
| "-jvm-default" to listOf("enable", "no-compatibility", "disable"), | ||
| "-Xassertions" to listOf("always-enable", "always-disable", "jvm", "legacy"), | ||
| "-Xlambdas" to listOf("class", "indy"), | ||
| "-Xsam-conversions" to listOf("class", "indy"), |
There was a problem hiding this comment.
The reflection approach could pull information from the valueDescription.
Does the json expose something similar?
| True: ["-Xallow-result-return-type"], | ||
| }, | ||
| ), | ||
| "x_annotation_default_target": struct( |
There was a problem hiding this comment.
We seem to have lost the common compiler arguments, like this one.
|
@agluszak if we are making incompatible release anyway (Boolean -> String), it's worth comparing compiler arguments before and after, including default values, so it won't be a complete surprise (and document those changes in changelog). |
3 participants
Implement automatic generation of kotlinc options using the official kotlin-compiler-arguments-description artifact from JetBrains.
Changes:
Closes #1444