-
Notifications
You must be signed in to change notification settings - Fork 46
[feature] Go support #507
base: master
Are you sure you want to change the base?
[feature] Go support #507
Conversation
MODULE.bazel
Outdated
@@ -16,6 +16,7 @@ bazel_dep(name = "rules_proto", version = "5.3.0-21.7") | |||
bazel_dep(name = "rules_cc", version = "0.0.9") | |||
bazel_dep(name = "platforms", version = "0.0.8") | |||
bazel_dep(name = "rules_python", version = "0.27.1") | |||
bazel_dep(name = "rules_go", version = "0.43.0") |
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.
i think you dont need it
# Conflicts: # MODULE.bazel
# Conflicts: # protocol/src/main/kotlin/org/jetbrains/bsp/utils/Extractors.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/bsp/managers/BazelBspLanguageExtensionsGenerator.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/common/ServerContainer.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/languages/LanguagePluginsService.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/model/Language.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/proto/bsp_target_info.proto # server/src/test/kotlin/org/jetbrains/bsp/bazel/server/sync/languages/LanguagePluginServiceTest.kt
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.
some small things, so far looks good
aspects/rules/go/go_info.bzl
Outdated
load("//aspects:utils/utils.bzl", "create_proto", "create_struct", "file_location") | ||
load("@io_bazel_rules_go//go:def.bzl", "go_context") | ||
|
||
def extract_sdk(ctx): |
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.
let's put an _
to mark that's "private" and put the definition below its usage - so just put it under the extract_go_info
aspects/rules/go/go_info.bzl
Outdated
return None | ||
return file_location(go.sdk.go) | ||
|
||
def extract_go_info(target, ctx, **kwargs): |
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.
you also need to check is it a go target (we usually check is there a language provider in the target, if not then we return None, None
)
aspects/rules/go/go_info.bzl
Outdated
return file_location(go.sdk.go) | ||
|
||
def extract_go_info(target, ctx, **kwargs): | ||
importpath = getattr(ctx.rule.attr, "importpath", []) |
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.
importpath is a string, so the default fallback should be an empty string (or none)
|
||
data class GoBuildTarget( | ||
val sdkHomePath: URI?, | ||
val importPath: String?, |
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.
question is: can target have no import path?
data class GoModule ( | ||
val sdkHomePath: URI?, | ||
val importPath: String?, | ||
): LanguageData |
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.
new line
val goSdkDir = goBinaryPath.parent.parent | ||
goSdkDir.toUri() | ||
} | ||
} |
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.
new line
plus you can also rebase it and try to add e2e tests |
# Conflicts: # MODULE.bazel.lock # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/bsp/managers/BazelBspLanguageExtensionsGenerator.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/BazelProjectMapper.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/TargetInfoReader.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/model/Language.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/proto/bsp_target_info.proto
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.
small thing
server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/BazelProjectMapper.kt
Show resolved
Hide resolved
…k/go-support # Conflicts: # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/BazelBspServer.kt # server/src/test/kotlin/org/jetbrains/bsp/bazel/server/sync/languages/LanguagePluginServiceTest.kt
# Conflicts: # MODULE.bazel.lock # server/src/test/kotlin/org/jetbrains/bsp/bazel/server/bsp/managers/BazelBspLanguageExtensionsGeneratorTest.kt
# Conflicts: # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/BazelProjectMapper.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/BspProjectMapper.kt
…support # Conflicts: # .bazelrc # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/BazelProjectMapper.kt
# Conflicts: # .bazelrc # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/BazelProjectMapper.kt # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/TargetInfoReader.kt
# Conflicts: # .bazelrc # aspects/rules/go/go_info.bzl.template # server/src/main/kotlin/org/jetbrains/bsp/bazel/server/sync/BazelProjectMapper.kt
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.
looks reallny nice, some minor comments
val goImportPath: String? = "", | ||
val goRoot: URI? = URI(""), |
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.
can be null since it's nullable
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.
makes sense
val goImportPath: String? = "", | ||
val goRoot: URI? = URI(""), |
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.
can be null as ewll
importpath = go_archive.data.importpath | ||
sdk_home_path = _extract_sdk(ctx) | ||
|
||
go_target_info = create_struct( |
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.
pls dont forget about embeds
BAZEL-749