rules_closure icon indicating copy to clipboard operation
rules_closure copied to clipboard

Stop given a depset as the first unnamed argument to the depset() con…

Open Yexo opened this issue 4 years ago • 3 comments

…structor.

This is in preparation for flipping the Bazel flag --incompatible_disable_depset_items to true, making it an error to use the "items" parameter. See https://github.com/bazelbuild/bazel/issues/9017 for details.

Yexo avatar Mar 30 '20 13:03 Yexo

Requiring a sequence instead of also accepting a depset for exports and internal_descriptors is a breaking change that I think we should try to avoid.

Looking at https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/457, it seems like rules_closure is compatible with --incompatible_disable_depset_items. Can you give more insights about why this is necessary?

Yannic avatar Mar 30 '20 13:03 Yannic

Requiring a sequence instead of also accepting a depset for exports and internal_descriptors is a breaking change that I think we should try to avoid.

It's only a change to _closure_js_library_impl, which is internal to this .bzl file and only used twice: First is from create_closure_js_library which uses neither exports nor internal_descriptiors, so this change is a no-op there. Second usage is from _closure_js_library which already passes in a list for both.

Looking at https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/457, it seems like rules_closure is compatible with --incompatible_disable_depset_items. Can you give more insights about why this is necessary?

I guess this is not strictly necesary. A recent warning I added to buildifier (https://github.com/bazelbuild/buildtools/commit/6827138c86919697c98133b84678b9176225ba8f) flagged this file as a violation (incorrectly, due to using mixed types) and while evaluating whether the warning was correct or not I figured I could simplify the code a bit.

Yexo avatar Mar 30 '20 13:03 Yexo

There hasn't been any activity in this PR for very long time. Please let me know if there are any intentions to pursue this change. Otherwise I will close it. Thanks.

gkdn avatar Aug 06 '22 01:08 gkdn