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

WIP: Use JavaInfo instead of .java. #1075

Closed
wants to merge 3 commits into from

Conversation

iirina
Copy link

@iirina iirina commented Aug 12, 2019

This PR only builds with bazel + bazelbuild/bazel#9135.

@iirina
Copy link
Author

iirina commented Aug 12, 2019

@brendandouglas This PR migrates from .java to JavaInfo in order to flip --incompatible_disallow_legacy_java_provider in bazel 1.0.

This PR doesn't build with bazel right now because it needs bazelbuild/bazel#9135 to be submitted. We are considering to cherry pick bazelbuild/bazel#9135 into bazel 0.29 if it allows us to flip the incompatbile flag (intellij is the only project that requires bazel changes).

I tested it locally running

~/my-bazel test //aspect/testing/... --incompatible_disallow_legacy_java_provider

Let me know what you think.

@brendandouglas
Copy link
Contributor

My main concern is internal usage of the legacy provider. Are any internal rules exposing different information in the "java" and JavaInfo providers, or not exposing the JavaInfo provider at all?

@brendandouglas
Copy link
Contributor

A related question -- is this migration being done externally first because of some internal-only complications? If so, that would block changes here.

@iirina
Copy link
Author

iirina commented Aug 12, 2019

My main goal right now is to flip the flag for bazel 1.0, this is why I started with external changes. There are no known internal issues with .java vs JavaInfo. The internal rules expose the same providers as the external ones. I can move this PR to an internal cl to make sure we don't accidentally break internal projects, but apart from that I wanted to get your opinion on the matter first.

@brendandouglas
Copy link
Contributor

Thanks. I haven't been keeping up with the changes to JavaInfo over the last year or two. Last I checked, which was a long time ago, some required providers weren't exposed in JavaInfo. Looks like that's all been fixed now though, so there may be nothing blocking this.

An internal CL would be best thanks, GitHub isn't the source of truth for this project. We'll want to run some manual testing before making this change, but otherwise it seems fine.

@iirina
Copy link
Author

iirina commented Aug 12, 2019

Sounds good, thanks! I'll close this and send an internal change after I expose JavaInfo to java proto aspects in blaze.

@iirina iirina closed this Aug 12, 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.

3 participants