-
Notifications
You must be signed in to change notification settings - Fork 277
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
Fix remaining incompatible changes warnings #480
Conversation
please take a look @ittaiz |
scala/scala.bzl
Outdated
|
||
|
||
def _expand_location(ctx, flags): | ||
return [ctx.expand_location(f, ctx.attr.data) for f in flags] | ||
|
||
def _sort_join_path(args, sep=","): | ||
return sep.join(sorted([f.path for f in args])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sorting is not needed. Depsets have stable orderings so we don't need to sort again to achieve that.
https://docs.bazel.build/versions/master/skylark/lib/depset.html
okay, I think this is finally ready to go. I added tests to check with all_incompatible_changes, and I test it in strict deps mode and regular mode. I think we are finally in good shape here... If we can use modern providers everywhere and not break intellij we may be all caught up. |
cc @andyscott |
@ittaiz can we merge this? I think it is ready to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! I had a pending comment about why did you keep the sort but I just saw you removed it :)
Fixes: ERROR: test/proto/BUILD:36:1: in scala_proto_srcjar rule //test/proto:test_proto_java_conversions_srcjar: Traceback (most recent call last): File "test/proto/BUILD", line 36 scala_proto_srcjar(name = 'test_proto_java_conversions_srcjar') File "scala_proto/scala_proto.bzl", line 332, in _gen_proto_srcjar_impl transitive_proto_paths += target.proto.transitive_proto_path `+` operator on a depset is forbidden. See https://docs.bazel.build/versions/master/skylark/depsets.html for recommendations. Use --incompatible_depset_union=false to temporarily disable this check. See also bazelbuild#480
* Fix remaining incompatible changes warnings * add tests, fix test for strict deps
Fixes: ERROR: test/proto/BUILD:36:1: in scala_proto_srcjar rule //test/proto:test_proto_java_conversions_srcjar: Traceback (most recent call last): File "test/proto/BUILD", line 36 scala_proto_srcjar(name = 'test_proto_java_conversions_srcjar') File "scala_proto/scala_proto.bzl", line 332, in _gen_proto_srcjar_impl transitive_proto_paths += target.proto.transitive_proto_path `+` operator on a depset is forbidden. See https://docs.bazel.build/versions/master/skylark/depsets.html for recommendations. Use --incompatible_depset_union=false to temporarily disable this check. See also bazelbuild#480
This fixes the remaining issues to resolve #430. With this, I can pass all tests using
--all_incompatible_changes
.We test both strict deps and without strict deps to make sure we can pass with the all_incompatible_changes. Since the build should be exactly the same, the cache works when things pass, so it doesn't really increase CI time.