Conversation
UebelAndre
left a comment
UebelAndre
left a comment
There was a problem hiding this comment.
Can you add a test, demonstrating how the TemplateVariableInfo would be consumed?
Additionally, would you be able to move the str(Label()) stuff into a separate PR so the required changes to the toolchain for the new functionality is clearer?
Happy to do those things, will update shortly. |
uhthomas
force-pushed
the
1377-2
branch
2 times, most recently
from
4a34748 to
25b7b6f
Compare
|
@UebelAndre I've removed the |
uhthomas
force-pushed
the
1377-2
branch
3 times, most recently
from
e81c828 to
4ff6367
Compare
UebelAndre
left a comment
UebelAndre
left a comment
There was a problem hiding this comment.
I love all the cleanup but could the provider defnitions (with the exception of rust_toolchain) also be moved to a separate PR? To keep the implementation of TemplateVariableInfo as concise as possible?
| name: Rust TemplateVariableInfo Tests | ||
| platform: ubuntu2004 | ||
| test_targets: | ||
| - "//test/rust_toolchain_template_variable_info:rust_toolchain_template_info_test" |
There was a problem hiding this comment.
Wouldn't this test be picked up by existing jobs?
| @@ -0,0 +1,26 @@ | |||
| """A module which defines rules for testing TemplateVariableInfo""" | |||
There was a problem hiding this comment.
There's already similar tests in //test/unit/toolchain_make_variables/toolchain_make_variables_test.bzl, could you instead expand the pattern there (specifically just using the starlark unittest framework) to cover the new cases?
There was a problem hiding this comment.
Ah, yes. Will merge them. Thanks!
There was a problem hiding this comment.
Only trouble is bazelbuild/bazel-skylib#375 sort of blocks it.
There was a problem hiding this comment.
Do you need to write a rule that consumes the toolchain? Is the target you wrote for the test the thing you want to do? Have some target with:
env = {
"CARGO": "$(CARGO)",
"RUSTC": "$(RUSTC)",
}That should be testable in a similar way to the current tests. You're not transitioning these targets so couldn't you also use current_rust_toolchain to find values to match against if you wanted to test environment variables for the action?
There was a problem hiding this comment.
If a unit test is hard because of issues in skylib, let's just use an integration test?
I filed uhthomas#1 with an example - WDYT?
There was a problem hiding this comment.
Even better, some usages of "$(rootpath @rules_rust//rust/toolchain:current_cargo_files)" should be replaced with the template variable info.
rules_rust/crate_universe/BUILD.bazel
Line 105 in 16175c8
There was a problem hiding this comment.
#1412 (comment) I think this should be a separate PR but sounds reasonable.
@uhthomas Can you take another look? I've updated the tests.
There was a problem hiding this comment.
Seems like the functionality you want is only supported in Bazel >5?
https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/0181729b-eb6e-47cf-8dc8-e291aac72a12/test/unit/toolchain_make_variables/rust_toolchain_make_variable_expansion_test/test.log
There was a problem hiding this comment.
No? It outputs a path just fine, though the path seems to be different?
In test _rust_toolchain_make_variable_expansion_test from //test/unit/toolchain_make_variables:toolchain_make_variables_test.bzl: Expected env[ENV_VAR_CARGO] to be equal to 'bazel-out/k8-fastbuild/bin/external/rust_linux_x86_64/toolchain_for_x86_64-unknown-linux-gnu_impl/bin/cargo', got 'bazel-out/host/bin/external/rust_linux_x86_64/toolchain_for_x86_64-unknown-linux-gnu_impl/bin/cargo'
-bazel-out/k8-fastbuild/bin/external/rust_linux_x86_64/toolchain_for_x86_64-unknown-linux-gnu_impl/bin/cargo|
+bazel-out/host/bin/external/rust_linux_x86_64/toolchain_for_x86_64-unknown-linux-gnu_impl/bin/cargoThere was a problem hiding this comment.
Is this something you'd be able to pickup and find a solution for? It's also odd to me that the tests passed on all other jobs but not the Min Bazel Version ones
uhthomas
changed the title
| @@ -0,0 +1,26 @@ | |||
| """A module which defines rules for testing TemplateVariableInfo""" | |||
There was a problem hiding this comment.
Do you need to write a rule that consumes the toolchain? Is the target you wrote for the test the thing you want to do? Have some target with:
env = {
"CARGO": "$(CARGO)",
"RUSTC": "$(RUSTC)",
}That should be testable in a similar way to the current tests. You're not transitioning these targets so couldn't you also use current_rust_toolchain to find values to match against if you wanted to test environment variables for the action?
|
@illicitonion may have some good insight here. |
|
I think all the implementation here was merged in #1416. If there's anything I missed on that PR I'm happy to review another, but hopefully you have the features you were hoping to get now 😄 |
|
Amazing! Thanks |
3 participants
Fixes #1377 (comment)