Various starlark cleanups #4010
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| @@ -515,7 +515,7 @@ def _sdk_build_file(ctx, platform, version, experiments): | |||
| "{goarch}": goarch, | |||
| "{exe}": ".exe" if goos == "windows" else "", | |||
| "{version}": version, | |||
| "{experiments}": repr(experiments), | |||
| "{experiments}": ",".join(experiments), | |||
There was a problem hiding this comment.
Does this provide a meaningful performance boost? I'm doubtful of that as there shouldn't be that many go_sdk repos in a given project. Since repr is more obviously correct, I would prefer to keep it.
There was a problem hiding this comment.
It allows this simplification which affects every go action
- "GOEXPERIMENT": ",".join(toolchain.sdk.experiments),
+ "GOEXPERIMENT": toolchain.sdk.experiments,
go/private/actions/compilepkg.bzl
Outdated
| @@ -82,7 +82,7 @@ def emit_compilepkg( | |||
| if bool(nogo) != bool(out_facts): | |||
| fail("nogo must be specified if and only if out_facts is specified") | |||
|
|
|||
| inputs = (sources + embedsrcs + [go.package_list] + | |||
| inputs = (sources + embedsrcs + [go.sdk.package_list] + | |||
There was a problem hiding this comment.
Only vaguely related: I wonder what we could save if we used depsets here.
There was a problem hiding this comment.
we can try in a follow-up :)
| @@ -578,14 +570,9 @@ def go_context(ctx, attr = None): | |||
| root = goroot, | |||
| go = binary, | |||
| stdlib = stdlib, | |||
There was a problem hiding this comment.
Yay for making this smaller. It's technically public API, but I don't want to view it as such.
| def get_mode(ctx, go_toolchain, cgo_context_info, go_config_info): | ||
| static = _ternary(go_config_info.static if go_config_info else "off") | ||
| static = go_config_info.static if go_config_info else False |
There was a problem hiding this comment.
I believe these values come from //go:config:static so they are already bools, though it's certainly possible that I am not fully following how this works. Tests seem to pass, not sure how our coverage is
| @@ -90,6 +94,8 @@ def _proto_library_to_source(_go, attr, source, merge): | |||
| merge(source, compiler[GoSource]) | |||
|
|
|||
| def _go_proto_library_impl(ctx): | |||
| check_importpaths(ctx) | |||
There was a problem hiding this comment.
Yeah so one of the things bothering me is that there are some differences between the library (base, proto, tool) and binary/test attributes/codepaths, but they both funnel into go_context. Then we end up having a bunch of getattr/hasattr and fallbacks to handle both of these. I wonder if it would be cleaner to have a dedicated lib_context and bin_context that have a shared shape?
Absent that, this change hoists check_importpath calls to the library entrypoints, because these attrs don't exist on binaries, and this allows simplifying the check function to avoid hasattr which speeds it up.
Without:
With:
|
Added another commit from testing in @jbedard's private repo; After: |
|
I'm going to break this up into multiple PRs, starting with #4014 |
What type of PR is this?
> Other
What does this PR do? Why is it needed?
Cleans up various Stalark inefficiencies. This saves around .5s analysis time in our repo, which does not have too much Go code yet. It is likely more noticeable in repos that have more go code
Which issues(s) does this PR fix?
Fixes #
Other notes for review