Skip to content
This repository has been archived by the owner on Aug 5, 2024. It is now read-only.

[fix] add support for Scala SDK provided by rules_jvm_external #403

Merged
merged 4 commits into from
Jun 19, 2023

Conversation

dorranh
Copy link
Contributor

@dorranh dorranh commented May 9, 2023

While bazel-bsp is able to properly pick up Scala SDK JAR files provided by the rules_scala default toolchain, it currently fails to identify the SDK version when the scala toolchain is configured to use JARs provided by rules_jvm_external. This occurs because rules_jvm_external prepends a processed_ prefix to the jar name (source).

This PR addresses the above issue by extending the regex in ScalaSdkResolver to allow for an optional processed_ prefix.

Updated the regex used for determing Scala SDK versions to
allow the jars to optionally include a 'processed_' prefix.
This allows jars provided by rules_jvm_external to be
properly matched by ScalaSdkResolver.
Copy link
Member

@abrams27 abrams27 left a comment

Choose a reason for hiding this comment

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

thanks a lot, please update changelog as well

@abrams27 abrams27 changed the title Add support for Scala SDK provided by rules_jvm_external [fix] add support for Scala SDK provided by rules_jvm_external May 10, 2023
@jastice jastice requested a review from abrams27 May 15, 2023 07:28
@dorranh
Copy link
Contributor Author

dorranh commented May 15, 2023

Hi @abrams27, I've updated the CHANGELOG (had missed the contributor docs) and have also attempted to fix the formatting error CI was complaining about.

@lukaszwawrzyk
Copy link
Contributor

I recommend to disable stamping (see bazel-contrib/rules_jvm_external#786) Bsp will also have trouble with detecting right jars and source jars for libraries with stamping enabled.

@abrams27
Copy link
Member

@lukaszwawrzyk does it mean that this shouldn't be merged or maybe we need something more?

@lukaszwawrzyk
Copy link
Contributor

I mean, for purpose of valid just a valid scala sdk it might be fine. On the other hand I am not sure if it is worth maintaining.
I have seen symptoms of this issue, it resulted in header_ jars in the libraries instead of full jars with code (also heuristics for matching source jars was not working). I was considering fixing it in bazel-bsp and bsp plugin (it has source matching heuristic). I felt that it is too much for such special casing. I also feel that such random breaking change in rules_scala that adds processed_ prefix to all jars in the classpath is not a smart move and should be reverted.
I have this issue fixed in my repo by just adding one line to .bazelrc and I left this out on the bsp side. I hope that rules_scala will undo this (maybe they did already, I didn't check it).

@dorranh
Copy link
Contributor Author

dorranh commented May 16, 2023

Hi @lukaszwawrzyk, thanks for the additional background. I've only recently picked rules_scala back up after a couple of year hiatus.

From a purely UX perspective, my opinion is that bazel-bsp should either work more or less "out of the box" with recent rules_scala versions or very clearly lay out in the docs that the above workaround is required when using more recent versions of rules_scala. While I have not yet run into the issues you mention, the changes in this PR don't include any end to end tests to check for those kinds of cases. If there is interest in landing this PR then I would be happy to look into adding additional tests if you have some reproducible examples you could share.

@lukaszwawrzyk
Copy link
Contributor

To work out of the box, I think it would just need to add this flag to the build. This is not the best, because builds from the command line would invalidate the cache. I guess the documentation is the way to go here. WDYT @abrams27

@abrams27
Copy link
Member

hmm in the perfect scenario we'd like to support such cases out of the box as well, but in this release cycle (till end of the july) scala is not our priority so we dont have enough resources to investigate it deeply; imo we should support it anyway, but maybe not now since u are not sure about it @lukaszwawrzyk - we can include it in the readme for now

@abrams27
Copy link
Member

hi, sorry for such delay, thanks a lot for the PR once again!

pls rebase it and we are good to go

@abrams27
Copy link
Member

okey i managed to fix the changelog

@dorranh
Copy link
Contributor Author

dorranh commented Jun 16, 2023

Thanks for the assist, @abrams27! I unfortunately haven't had the cycles for this PR this week.

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

Successfully merging this pull request may close these issues.

3 participants