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.

Sign up for GitHub

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

Jump to bottom

Remove legacy struct providers #1157

Merged
merged 3 commits into from
Apr 26, 2024
Merged

Remove legacy struct providers #1157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d4ea43
Remove legacy struct providers in main repo
comius Apr 5, 2024
2852ecf
Change _toolchain to provide Kotlin toolchain info
comius Apr 19, 2024
93bbd24
Add back original _toolchain and use _kt_toolchain for Kotlin
comius Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 34 additions & 40 deletions kotlin/internal/jvm/impl.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,22 @@ def _make_providers(ctx, providers, transitive_files = depset(order = "default")
files = [ctx.outputs.jar]
if providers.java.outputs.jdeps:
files.append(providers.java.outputs.jdeps)
return struct(
kt = providers.kt,

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct, this would cause issues to IntelliJ integration. Struct providers have been deprecated for a long time, and this is one of the very last uses that I detected on Bazel downstream tests. (There are other failures, but mostly due to the use of old versions of other dependencies).

IntelliJ aspect was also until recently using a struct provider for Android rules, that was removed recently. I'll procure a fix for IntelliJ/Kotlin integration. Do you think we should block this PR until that is released?

I'd really like to flip the flag in Bazel @Head to disable struct providers and we're now in the tail of the tail of cleaning up the uses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IntelliJ aspect was also until recently using a struct provider for Android rules, that was removed recently. I'll procure a fix for IntelliJ/Kotlin integration. Do you think we should block this PR until that is released?

I'm all for merging this as long as there is a plan in place to fix the Intellij side that they are bought into merging. It would be unfortunate if we drop the struct here and then the Intellij team doesn't merge a similar fix, leaving Kotlin users in limbo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's another problem. I started looking how to fix Intellij. rules_kotlin is using KtJvmInfo here. However, the provider is only exposed through @rules_kotlin//kotlin/internal:defs.bzl. Is it ok to depend on that in IntelliJ, or should we first move it to a more public location?

I also wonder why couldn't IntelliJ use just JavaInfo provider. It seems that there's less information in there than in KtJvmInfo (bazelbuild/intellij#1202). But using JavaInfo would mean nothing needs to be changed in IntelliJ. Maybe we can put more information there?

cc @jin

Copy link
Collaborator

Choose a reason for hiding this comment

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

There might be some logic that depends on knowing that a target is a "kotlin" in order to enable some Kotlin specific features inside Intellij. I believe this is also the case for Android targets.

If that's not true then in theory it should be fine to just check that final JavaInfo provider. I'd defer to the Intellij team for these questions though since they wrote and maintain the Kotlin integration in the aswb/ijwb plugins.

Copy link
Member

Choose a reason for hiding this comment

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

It's been a while since I looked at this, so memory's bit hazy.

Just by skimming the code, ISTM that as long as we can provide IntelliJ here with a "marker provider" about a target's Kotlin-ness, then consolidate on JavaInfo on everything else, things should continue to work?

Copy link
Contributor Author

@comius comius Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, what I figured out is that JavaInfo already provides all the information needed. This was done through the years with improvements to Java compilation process. (Change that made it seems to be 2be75c0)

For the toolchains part, a way to expose it to IntelliJ is via _kt_toolchain attribute, which provides language information. We are doing almost the same thing internally.

To support reading in IntelliJ I prepared:
bazelbuild/intellij#6394
(the IntelliJ plugin CI is ATM broken because of a different change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bazelbuild/intellij#6394 was merged.

@Bencodes friendly ping

providers = [
providers.java,
providers.kt,
providers.instrumented_files,
DefaultInfo(
files = depset(files),
runfiles = ctx.runfiles(
# explicitly include data files, otherwise they appear to be missing
files = ctx.files.data,
transitive_files = transitive_files,
# continue to use collect_default until proper transitive data collecting is
# implmented.
collect_default = True,
),
return [
providers.java,
providers.kt,
providers.instrumented_files,
DefaultInfo(
files = depset(files),
runfiles = ctx.runfiles(
# explicitly include data files, otherwise they appear to be missing
files = ctx.files.data,
transitive_files = transitive_files,
# continue to use collect_default until proper transitive data collecting is
# implmented.
collect_default = True,
),
] + list(additional_providers),
)
),
] + list(additional_providers)

def _write_launcher_action(ctx, rjars, main_class, jvm_flags):
"""Macro that writes out a launcher script shell script.
Expand Down Expand Up @@ -193,28 +190,25 @@ def kt_jvm_import_impl(ctx):
),
)

return struct(
kt = kt_info,
providers = [
DefaultInfo(
files = depset(direct = [artifact.class_jar]),
runfiles = ctx.runfiles(
# Append class jar with the optional sources jar
files = [artifact.class_jar] + [artifact.source_jar] if artifact.source_jar else [],
).merge_all([d[DefaultInfo].default_runfiles for d in ctx.attr.deps]),
),
JavaInfo(
output_jar = artifact.class_jar,
compile_jar = artifact.class_jar,
source_jar = artifact.source_jar,
runtime_deps = [dep[JavaInfo] for dep in ctx.attr.runtime_deps if JavaInfo in dep],
deps = [dep[JavaInfo] for dep in ctx.attr.deps if JavaInfo in dep],
exports = [d[JavaInfo] for d in getattr(ctx.attr, "exports", [])],
neverlink = getattr(ctx.attr, "neverlink", False),
),
kt_info,
],
)
return [
DefaultInfo(
files = depset(direct = [artifact.class_jar]),
runfiles = ctx.runfiles(
# Append class jar with the optional sources jar
files = [artifact.class_jar] + [artifact.source_jar] if artifact.source_jar else [],
).merge_all([d[DefaultInfo].default_runfiles for d in ctx.attr.deps]),
),
JavaInfo(
output_jar = artifact.class_jar,
compile_jar = artifact.class_jar,
source_jar = artifact.source_jar,
runtime_deps = [dep[JavaInfo] for dep in ctx.attr.runtime_deps if JavaInfo in dep],
deps = [dep[JavaInfo] for dep in ctx.attr.deps if JavaInfo in dep],
exports = [d[JavaInfo] for d in getattr(ctx.attr, "exports", [])],
neverlink = getattr(ctx.attr, "neverlink", False),
),
kt_info,
]

def kt_jvm_library_impl(ctx):
if ctx.attr.neverlink and ctx.attr.runtime_deps:
Expand Down
6 changes: 6 additions & 0 deletions kotlin/internal/jvm/jvm.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ _implicit_deps = {
default = Label("//kotlin/compiler:kotlin-stdlib"),
cfg = "target",
),
"_kt_toolchain": attr.label(
doc = """The Kotlin toolchain. it's only purpose is to enable the Intellij
to discover Kotlin language version""",
default = Label("//kotlin/internal:default_toolchain_impl"),
cfg = "target",
),
"_java_toolchain": attr.label(
default = Label("@bazel_tools//tools/jdk:current_java_toolchain"),
),
Expand Down
30 changes: 14 additions & 16 deletions kotlin/internal/utils/generate_jvm_service.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,20 @@ def _generate_jvm_service_impl(ctx):
arguments = [zipper_args],
progress_message = "JVM service info jar for %%{label}",
)
return struct(
providers = [
JavaInfo(
output_jar = jar,
compile_jar = jar,
source_jar = jar,
runtime_deps = [],
exports = [],
neverlink = False,
),
DefaultInfo(
files = depset([jar]),
runfiles = ctx.runfiles(files = [jar]),
),
],
)
return [
JavaInfo(
output_jar = jar,
compile_jar = jar,
source_jar = jar,
runtime_deps = [],
exports = [],
neverlink = False,
),
DefaultInfo(
files = depset([jar]),
runfiles = ctx.runfiles(files = [jar]),
),
]

def _write_service_file(ctx, srv, impls):
f = ctx.actions.declare_file(ctx.label.name + "/" + srv)
Expand Down
6 changes: 1 addition & 5 deletions src/main/starlark/core/options/opts.javac.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,7 @@ _JOPTS = {
}

def _javac_options_impl(ctx):
return struct(
providers = [
JavacOptions(**{n: getattr(ctx.attr, n, None) for n in _JOPTS}),
],
)
return [JavacOptions(**{n: getattr(ctx.attr, n, None) for n in _JOPTS})]

JavacOptions = provider(
fields = {
Expand Down
6 changes: 1 addition & 5 deletions src/main/starlark/core/options/opts.kotlinc.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,11 +362,7 @@ KotlincOptions = provider(
)

def _kotlinc_options_impl(ctx):
return struct(
providers = [
KotlincOptions(**{n: getattr(ctx.attr, n, None) for n in _KOPTS}),
],
)
return [KotlincOptions(**{n: getattr(ctx.attr, n, None) for n in _KOPTS})]

kt_kotlinc_options = rule(
implementation = _kotlinc_options_impl,
Expand Down