Closed CauhxMilloy closed 7 months ago
Thank you for offering to contribute. I always get pleasantly surprised when someone seems to find this work useful. I'll need some time to read through your reasoning, and will get back to you.
Totally, take your time. I put this issue out (instead of just sending a PR) because I wanted to take the time and chat with you about what the better approach ought to be.
When I modified the expansion logic before (same person, different GH account, btw), I had specific scenarios around using $(location ...)
. I have even more scenarios now. Unfortunately, only the internal (Java) expansion logic handles it properly and it exposes no API to Skylark for doing it (easily) -- the exposed APIs (through ctx
) aren't super great (more detail in the linked PR for Skylib).
If you wanted more proof of concept BUILD/bzl files, or if had questions and wanted to chat more, that's cool too. Let me know.
Thanks having this repo and always be so responsive when I want to add stuff in!
I went ahead and cleaned up the 2 impls and pushed them to branches on my fork. Feel free to check them out and let me know how you feel.
.bzl
approach: https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithBzl
sh_test
approach: https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithShTest
(all changes are in a single commit on those respective branches)
I think the .bzl
approach is better (especially with the logic added directly into this repo). But I think the sh_test
approach is fine, based on preference.
Thanks. After taking a look, I suspect that pushing this logic upstream as much as we can is probably the best route.
Therefore I'd vote for the bzl
approach, send the PR over when you have the time.
Cool, put up https://github.com/filmil/bazel-bats/pull/28. Thanks!
Currently in
_bats_test_impl()
, environment variables (from theenv
attribute) are expanded in a two step process. First, any$(location ...)
(or similar) are expanded (usingctx.expand_location()
), then any variables are expanded (usingctx.expand_make_variables()
).This covers many situations. However, if any variables expand to
$(location ...)
(or similar), an unrecoverable error is thrown (exception from Java). This level of recursive expansion is supported by built-in rules. It is unfortunate that those APIs are not exposed to Skylark at all.For
bats_tests
targets to have the sameenv
support that built-in rules have, proper expansion logic must be used. This can be done in 2 ways (this GH issue is to discuss which is preferred).Use
sh_test
internallyThis option is done by using
sh_test
as the main internal target forbats_tests
. The existing custom rules (_bats_test_attrs
and_bats_with_bats_assert_test_attrs
) would still be generated, but the output executable is then wrapped in ash_test
. Theenv
attribute would be passed to thesh_test
-- and not to_bats_test_impl()
. This allows for the environment variables to be properly expanded by the built-in (Java) implementation for built-in rules. Other environment variables which are manually set in_bats_test_impl()
(e.g.TMPDIR
andPATH
) would remain expanding with their existing logic.Use fancier bzl expansion logic
This option is to have better expansion logic in Skylark which mimics (and optionally improves upon) the built-in expansion logic. For more reuse, I put up a PR to Skylib (https://github.com/bazelbuild/bazel-skylib/pull/486). This PR has more much detail about expansion logic in Bazel in the description. The logic in the PR also adds extra functionality beyond built-in expansion logic. This logic could either be referenced directly (adding Skylib as dep for bazel-bats) or added to a bzl file here to avoid extra deps.
I've implemented both of these approaches, and either way works fine. Let me know which you think would be better.