feat(): macros nodejs_binary_toolchains nodejs_test_toolchains input … #3132
Conversation
dymart
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
dymart
force-pushed
the
5.tool
branch
2 times, most recently
from
736e685 to
d1352a2
Compare
| @@ -250,8 +250,10 @@ fi | |||
| # when building the image as that will reflect the selected --platform. | |||
| node_tool_files = ctx.files.node[:] | |||
|
|
|||
| # this should be resolved the same as above | |||
| node_tool_files.extend(ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo.tool_files) | |||
| if ctx.attr.toolchain: | |||
There was a problem hiding this comment.
can reduce duplication by assigning to a local variable that holds either ctx.attr.toolchain or ctx.toolchains[default]
internal/node/node.bzl
Outdated
| @@ -658,6 +661,27 @@ def nodejs_binary_macro(name, **kwargs): | |||
| **kwargs | |||
| ) | |||
|
|
|||
| def nodejs_binary_toolchain_macro(name, toolchains = [], **kwargs): | |||
| """This extens nodejs binary and allows for multiple toolchains to be passed as a list and | |||
There was a problem hiding this comment.
nit: extends
maybe get a spell checker into your editor?
internal/node/node.bzl
Outdated
| @@ -658,6 +661,27 @@ def nodejs_binary_macro(name, **kwargs): | |||
| **kwargs | |||
| ) | |||
|
|
|||
| def nodejs_binary_toolchain_macro(name, toolchains = [], **kwargs): | |||
There was a problem hiding this comment.
I wonder if this is holding it's weight and worth putting in rules_nodejs code, rather than just put it in an example and let users maintain a little macro for this purpose. In all the life of the project I think we only heard of two users who want it
There was a problem hiding this comment.
I'm not sure what the outcome here should be. Should it switch to adding nodejs_binary to rules_nodejs then have an example there? Or just remove this macro and create an example that that shows users how to do it?
My only push back on not many users requesting it is, has there been many build tools that can make it this easy to run different toolchains on the same machine in parallel. Could be useful during upgrades to different node versions and allow better gradual migrations.
internal/node/test/BUILD.bazel
Outdated
| # bazel build //internal/node/test:main_multiple_0 | ||
| # bazel build //internal/node/test:main_multiple_1 | ||
| # the numbers are based on the order that the toolchains are passed to the rule | ||
| nodejs_binary_toolchains( |
There was a problem hiding this comment.
it might even be better to show it with a comprehension rather than a macro
[
nodejs_binary(name = "x" + tc, ...)
for tc in [...]
]
There was a problem hiding this comment.
So this would be in the case we decide to remove the macro mentioned in the above comment?
Is that the preferred outcome?
There was a problem hiding this comment.
We want to keep our maintenance burden in the upstream repo minimal, since it's not really funded. So any code maintained here ought to be broadly applicable. If only a couple users need it, then my instinct is it should live in their repo (we can have an example to help them).
There was a problem hiding this comment.
Sounds Good! thanks for the clarification
internal/node/test/BUILD.bazel
Outdated
| "@bazel_tools//src/conditions:darwin": "@node15_darwin_amd64//:node_toolchain", | ||
| "@bazel_tools//src/conditions:windows": "@node15_windows_amd64//:node_toolchain", | ||
| }), | ||
| select({ |
There was a problem hiding this comment.
seems like the two select's have the same content?
There was a problem hiding this comment.
Yes, I wasn't sure about the best way to check that the correct version of two different toolchain versions was being used from one test. So both toolchains are the same. I could also just run a normal test that doesn't check the version with two different toolchains. I'm open to suggestions
dymart
force-pushed
the
5.tool
branch
7 times, most recently
from
9804727 to
906f3ad
Compare
internal/node/test/BUILD.bazel
Outdated
| # bazel build //internal/node/test:main_multiple_loop_1 | ||
| # the numbers are based on the order that the toolchains are passed to the rule | ||
| [ | ||
| nodejs_binary( |
There was a problem hiding this comment.
duplicate targets to what is stamped out below?
//internal/node/test:main_multiple_loop_node16
//internal/node/test:main_multiple_loop_node15
?
There was a problem hiding this comment.
Yes they would be the same. Added to show different ways of doing the same thing. If there is a preferred way I can remove the other
There was a problem hiding this comment.
I think the for id, tc in zip( example with the clearer named targets is a better one so you can remove the enumerate one.
| nodejs_binary( | ||
| name = "main_multiple_loop_" + id, | ||
| entry_point = "binary_version.js", | ||
| node = tc, |
There was a problem hiding this comment.
Are you aiming to collapse node & toolchain into just one attr?
There was a problem hiding this comment.
I was hoping to get the use of toolchains added and then go back and clean this up. Originally it was behind a macro as well so a change collapsing the two wouldn't have been visible. But if we want to combine them now we could do that as well
There was a problem hiding this comment.
A follow-up of removing node in favor toolchain sounds good to me. Seems easier to do it that way and the downstream effects will be easier to see.
…multiple toolchains nodejs_binary or nodejs_test
|
Added test to verify node version to this pr for multiple versions |
…multiple toolchains nodejs_binary or nodejs_test
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Could only pass one toolchain to a rule
What is the new behavior?
can pass multiple toolchains to a rule and it will create a new target for each toolchain
Does this PR introduce a breaking change?
Other information