Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Yexo
Copy link

@Yexo Yexo commented Mar 30, 2020

…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 bazelbuild/bazel#9017 for details.

…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 bazelbuild/bazel#9017 for details.
@Yannic
Copy link
Contributor

Yannic commented Mar 30, 2020

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?

@Yexo
Copy link
Author

Yexo commented Mar 30, 2020

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 (bazelbuild/buildtools@6827138) 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.

@gkdn
Copy link
Collaborator

gkdn commented Aug 6, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants