Use a dummy toolchain context for rules that don't have one.#13162
Use a dummy toolchain context for rules that don't have one.#13162katre wants to merge 4 commits intobazelbuild:masterfrom katre:i12610-toolchain-crash
Conversation
|
|
||
| function test_toolchain_from_no_toolchain_rule() { | ||
| # Create a build_setting that tries to load a toolchain. | ||
| # build_settings cnanot actually have toolchains because they |
| cat >> demo/rule.bzl <<EOF | ||
| def _sample_impl(ctx): | ||
| info = ctx.toolchains["//:toolchain_type"] | ||
| if info: |
There was a problem hiding this comment.
if info != None:? To distinguish from other false-y values.
There was a problem hiding this comment.
Fixed in the unit test.
| expect_not_log "does not provide ToolchainTypeInfo" | ||
| } | ||
|
|
||
| function test_toolchain_from_no_toolchain_rule() { |
There was a problem hiding this comment.
What's the purpose of testing this at both the unit test and shell test levels?
There was a problem hiding this comment.
Good point, I wrote this first then added the other to debug, but we only need one. Removed the shell test.
| // Starlark rules are easier if this cannot be null, so return a no-op value instead. | ||
| return new ToolchainContextApi() { | ||
| @Override | ||
| public Object getIndex(StarlarkSemantics semantics, Object key) throws EvalException { |
There was a problem hiding this comment.
Nit: You can omit the throws declaration since this override doesn't throw. Same below.
| ResolvedToolchainContext toolchainContext = ruleContext.getToolchainContext(); | ||
| if (toolchainContext == null) { | ||
| // Starlark rules are easier if this cannot be null, so return a no-op value instead. | ||
| return new ToolchainContextApi() { |
There was a problem hiding this comment.
Optionally make this a singleton, but I doubt it'll matter much.
There was a problem hiding this comment.
I don't think there's enough benefit, this is fairly clear.
2 participants
Fixes #12610.