search

bazel-contrib / rules_go

Go rules for Bazel
Apache License 2.0
1.38k stars 656 forks source link

More go_context speedups #4058

Closed dzbarsky closed 1 month ago

dzbarsky commented 1 month ago

What type of PR is this? Starlark cleanup

What does this PR do? Why is it needed? After the previous cleanups, mode and GoConfigInfo are pretty similar, the only differences were link vs linkmode and goos/goarch were missing on GoConfigInfo. So we add the default ones from the toolchain, and this lets us use the GoConfigInfo in the majority of cases without having to copy all the properties to mode. Another option is to move the goos/goarch from mode to top-level properties on go_context, though this is potentially a breaking change. Having them on GoConfigInfo seems like a reasonable direction to move in anyway if we plan to use Bazel's platforms to control what we target.

This required aligning the linkmode name, which is good for consistency anyway.

We pass in goos/goarch to go_context since they are only set in terminal rules and we don't need to look them up for intermediate libs.

I also noticed getattr from the proto libs showing up in profiles, so we clean that up as well.

This speeds up go_context another 2x, saving 100ms in buildbuddy repo

Which issues(s) does this PR fix?

Fixes #

Other notes for review

dzbarsky commented 1 month ago

I suppose this is breaking either way since mode.link became mode.linkmode. Feels pretty minor, maybe we just mention that in release notes?

tyler-french commented 1 month ago

@dzbarsky @fmeum This one is causing failures too:


ERROR: /home/user/go-code/idl/../BUILD.bazel:13:20: in @@io_bazel_rules_go//proto:def.bzl%_go_proto_aspect aspect on _go_thriftrw_library rule //idl/code.uber.internal/config/object-config/host-agent:object_config_host_agent_go_thriftrw: 
Traceback (most recent call last):
        File "/home/user/.cache/bazel/_bazel_tfrench/b97476d719d716accead0f2d5b93104f/external/io_bazel_rules_go/proto/def.bzl", line 76, column 25, in _go_proto_aspect_impl
                importmap = attr.importmap,
Error: No attribute 'importmap' in attr. Make sure you declared a rule attribute with this name.
Available attributes: _action_listener, _config_dependencies, _default_deps, _go_context_data, applicable_licenses, aspect_hints, compatible_with, deprecation, deps, exec_compatible_with, exec_properties, expect_failure, features, generator_function, generator_location, generator_name, importpath, name, package, plugins, restricted_to, tags, target_compatible_with, testonly, thrift, thrift_root, thriftrw, toolchains, transitive_configs, visibility
dzbarsky commented 1 month ago

@tyler-french You can band-aid this by adding the following attributes to _go_thriftrw_library (this trio typically goes together on the rules):

        "importmap": attr.string(),
        "importpath_aliases": attr.string_list(),  # experimental, undocumented

I didn't think this part would be breaking, because this aspect is only applied to deps in go_proto_library and propagates along deps and embed. It's surprising to me that you have a thrift library that ends up as a dep of a proto library, I would expect it to be a tree of go_proto_libraries. Any idea what's going on here? (My guess was that maybe you have a proto plugin that depends on thrift, but I don't see how those deps would get the aspect applied...)

I supposed we could revert part of this PR to continue to support these as optional, though the direct property access is slightly faster than getattr. I'd like to understand what you're hitting though before we change that, since it's possible your graph is larger than you intended