Skip to content

Add targets that reproduce proto issues in 0.22.0 (and maybe earlier) #685

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

Merged
merged 3 commits into from
Feb 13, 2019

Conversation

beala-stripe
Copy link
Contributor

@beala-stripe beala-stripe commented Feb 8, 2019

This reproduces an issue in bazel 0.22.0 that breaks scalapb_proto_library. Issue to follow.

It's expected that the build will fail.

@johnynek
Copy link
Contributor

johnynek commented Feb 9, 2019

interesting this passes on 0.17... huh...

I have brought up many times that I think the proto rules need to improved #548 although I don't know if that will help here....

It seems like something at the bazel java skylark API may have changed.

@ittaiz
Copy link
Contributor

ittaiz commented Feb 9, 2019 via email

@beala-stripe
Copy link
Contributor Author

Thanks for the quick responses! I just wanted to call attention to the companion issue I created, which I believe explains the bazel API change that broke this: #687

The question in my mind is does this needs to be addressed in Bazel or in rules_scala? I don't have much context on the proto rules, but I'll keep digging on Monday.

This also adds tests for proto_library targets that use the proto_source_root attribute.
@beala-stripe
Copy link
Contributor Author

I pushed up a fix which mirrors the fix @lberki made here for the java rules: bazelbuild/bazel@af36058

This simply strips out the problematic proto_paths entry. This builds successfully in an internal repo, but none of our repos (afaik) exercise the proto rules very hard.

I've also added a test to targets that use the proto_source_root attribute, which was previously untested.

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. What do you think @ittaiz ?

Copy link
Contributor

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM sorry for the delay (still catching up from a long vacation)

@ittaiz ittaiz merged commit 817a6b2 into bazel-contrib:master Feb 13, 2019
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