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

Add support for an explicit srcjar for scala_import #486

Open
dflemstr opened this issue Apr 27, 2018 · 9 comments
Open

Add support for an explicit srcjar for scala_import #486

dflemstr opened this issue Apr 27, 2018 · 9 comments

Comments

@dflemstr
Copy link

It would be nice to have scala_import be on par with java_import in this regard, especially since scala_import_external suggests it should be supported.

I could probably take on fixing this if somebody could mentor me regarding a preferred solution.

@ittaiz
Copy link
Member

ittaiz commented Apr 27, 2018 via email

@dflemstr
Copy link
Author

Sounds good, I mostly wanted to create this issue to have something to track progress, it's not very urgent at all. If I happen to find the time to fix it on top of the old scala_import implementation on my own, would you accept that changeset anyway or still prefer to not have too many moving parts while the refactoring is going on?

@jjudd
Copy link
Contributor

jjudd commented Apr 27, 2018

We added srcjar support for Scala import in this commit: andyscott@a457c06, which is currently PR to this PR: #477

We currently use it with scala_import_external.

I'm not totally certain the status of that PR. We're busy rolling out Bazel for Scala to all of engineering internally over the next two weeks and have a bunch of forks that we'll need to clean up and get back onto master once we've migrated.

@ittaiz
Copy link
Member

ittaiz commented Apr 27, 2018 via email

@jjudd
Copy link
Contributor

jjudd commented Apr 27, 2018

If my PR gets merged to that PR, then it supports IntelliJ as well as rules_scala currently does. If we want, we could move forward with the PR now and then add the support to IntelliJ separately from that PR.

@johnynek
Copy link
Member

This dependency knot is a pain. I am losing state. I think bazel has to be updated for intellij to not lose state using only JavaInfo. So, that has to land, then we have to send the PR to the plugin, then we have to update locally.

@jjudd says we don't see a regression if we merge now. Can anyone confirm that? @andyscott @gkk-stripe ?

If we merge and regress on intellij it will be an issue for us at Stripe, and probably others.

@jjudd
Copy link
Contributor

jjudd commented Apr 27, 2018

@johnynek agreed. It's spaghetti right now.

We'll need this andyscott@a457c06 merged into @andyscott's branch before we merge if we want IntelliJ to continue working.

@andyscott
Copy link
Contributor

andyscott commented Apr 29, 2018 via email

@andyscott
Copy link
Contributor

See #487 for the sum of all lines of development.

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

No branches or pull requests

5 participants