Add support for expansions in expand_template #525
Conversation
keith
requested review from
brandjon,
tetromino,
comius,
hvadehra and
c-mita
as code owners
keith
force-pushed
the
ks/add-support-for-data-in-expand_template
branch
from
44814b2
to
1b19c61
Compare
This can be useful for encoding the paths to things in your expansions
keith
force-pushed
the
ks/add-support-for-data-in-expand_template
branch
from
1b19c61
to
d5ae39f
Compare
keith
changed the title
| @@ -16,10 +16,21 @@ | |||
| """ | |||
|
|
|||
| def _expand_template_impl(ctx): | |||
| expanded_substitutions = {} | |||
| for key, value in ctx.attr.substitutions.items(): | |||
| expanded_substitutions[key] = ctx.expand_make_variables( | |||
There was a problem hiding this comment.
ctx.expand_make_variable is marked as "Deprecated" in Bazel documentation. People before me marked it that way and I think we shouldn't introduce new uses.
Please remove it's use.
I believe it's implementable in Starlark, and skylib would be a nice location for such an implementation. (And we might already have one implementation in the C++ rules in builtins).
The rest of the PR seems fine.
There was a problem hiding this comment.
relevant convo https://groups.google.com/g/bazel-discuss/c/WEnG-WocaTc
so you're saying we should make a new function to do the same thing in skylib and use that here?
There was a problem hiding this comment.
Yes. I bumped bazelbuild/bazel#5859 to P3.
And I noticed also #486 which might already be doing that. Would you like to help in review?
There might be other PRs, I haven't gone through all of them yet.
This is useful for providing paths from data, and custom variables from toolchains