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 Scala support #113

Merged
merged 4 commits into from
Mar 31, 2024
Merged

Add Scala support #113

merged 4 commits into from
Mar 31, 2024

Conversation

Arthurm1
Copy link
Contributor

@Arthurm1 Arthurm1 commented Dec 5, 2023

Implements buildTarget/scalacOptions. Also returns ScalaBuildTarget instead of JvmBuildTarget if Scala has been configured for that target and passes back scala as a supported language.

I haven't separated out the implementation per language. I think a refactor would be better done in a single PR (not one where a new language is added). I'm not sure it's currently worth refactoring. Maybe when more languages are added?

Creating the scala compiler args using ZincScalaCompilerArgumentsGenerator is a bit verbose because of the class it requires to extract the setup from. It only actually needs 2 methods to be implemented. Another option would be to copy the code directly from ZincScalaCompilerArgumentsGenerator into SourceSetsModelBuilder. I thought this would be easier to maintain if the original is changed.

@jdneo
Copy link
Member

jdneo commented Dec 6, 2023

I'm not sure it's currently worth refactoring. Maybe when more languages are added?

To me, I would like to have a refactoring at current stage. Because:

  • The current GradleSourceSet.java has become a large object with a lot of fields.
  • If we defer the refactoring to the third language comes, things will be even more complex than now.

I'm sorry that this is a debt that I missed when implementing.

About the refactoring, I'm thinking that

  • We can have an extensibility that allows third-party plugin to provide their language build targets.
  • For the current embedded plugin, it can have different model builders taking care of their own language models.

We can do the second refactoring first to unblock the scala support.

The rough idea in my mind is:

  • the plugin will register the model builder according to the languageIds of the client capability.
  • The Gradle source set holds the common shared data.
  • The Gradle source set has an extension that holds language specific data. (like Scala version)

WDYT?

@Arthurm1
Copy link
Contributor Author

Arthurm1 commented Dec 6, 2023

the plugin will register the model builder according to the languageIds of the client capability.
The Gradle source set holds the common shared data.
The Gradle source set has an extension that holds language specific data. (like Scala version)

It sounds OK. I'm happy to code around your design, I'm just don't know whether that's the right direction as I haven't seen enough different language setups in Gradle to make a decision on splitting the design across languages.

One big advantage here is that I doubt anyone will be using this project as a library (it doesn't make sense to me to do that) so I think you can change the design/API as and when you like without affecting downstream projects.

Just some thoughts, not blockers...

  1. A single Build Target can have multiple language types so whatever these extensions are, the common gradle source set would need to be mapped to extensions 1->many, not 1->1. E.g scala can mix and match java and scala code within the same gradle project configuration. It would need both Scala extension info and Java extension info. So I wouldn't like to see the extension classes inherit from the common info class.

  2. We might want to build the common set regardless of whether the languages are supported by the BSP client. One example I can think of is that a multi-project might have mostly Java subprojects but have the test projects written in Kotlin. A Java only client could still run these tests without knowing anything about the Kotlin language and debug into the Java code. The BSP compile/run/test/debug requests are all language agnostic.

  3. Would this work nicely for Android? I've written some code to extract Gradle Android info before here. Annoyingly it ignores the Gradle sourceset structures and defines its own. So the common shared data class might not actually include sourcesets (but would include dependencies/modules/names etc).
    Would the Android extract be registered by language? I'm not sure that makes sense as the extracted sourcesets are still Java/Scala/Kotlin. Unless the new structure allows multiple ModelBuilders per language.
    AFAIK - you have to register your ToolingModelBuilder by the class it produces so I guess you'd have basic wrapper classes e.g. AndroidCommon, AndroidJava, AndroidScala, AndroidKotlin which just wrap the extension classes written for those languages?

@jdneo
Copy link
Member

jdneo commented Dec 6, 2023

Thank you @Arthurm1 for your input.

Here is my plan: I'll first make sure those opened PR get merged recently. At the meantime, I'll find some time think thoroughly about how to do the refactoring. We can continue the discussion here.

@jdneo
Copy link
Member

jdneo commented Dec 11, 2023

Annoyingly it ignores the Gradle sourceset structures and defines its own. So the common shared data class might not actually include sourcesets

I'm not very familiar with Android, which kind of property of the source set is not aligned when it comes to Android projects?

@jdneo
Copy link
Member

jdneo commented Dec 12, 2023

@Arthurm1 I now have a feeling that, maybe the BSP related types should be constructed at the plugin side, and the server is just simply get them from plugins and return them when different requests come.

Currently the server needs to know the definition of the source set and parse them to build target, etc... This makes trouble as you've mentioned, the concept of source set does not exist for Android, not to say other languages.

@Arthurm1
Copy link
Contributor Author

I now have a feeling that, maybe the BSP related types should be constructed at the plugin side

Maybe - I don't think that's a bad idea - I'm just not sure yet.

Android does have the concept of sourcesets - it just has it's own hierarchy.
So instead of org.gradle.api.tasks.SourceSet it has com.android.build.api.dsl.AndroidSourceSet

What I do think is that this data might have to be cached. Have you tried the plugin on a big project? e.g. Gradle itself. It takes about a minute to respond to build/initialize (which I think should be instant and is slow because the source sets are queried in response to this request but not yet needed). Then, after a compile, the source sets are all queried again. It would be good to delay the building of the sourcesets until the client requests them and do they need to be built again after every compile?

@jdneo
Copy link
Member

jdneo commented Dec 13, 2023

About the perf issue, let's discuss at #118.

Android does have the concept of sourcesets - it just has it's own hierarchy.
So instead of org.gradle.api.tasks.SourceSet it has com.android.build.api.dsl.AndroidSourceSet

Does that mean that for android project, we need a different way to fetch the information of the source set, but the properties of the source set are the same?

@Arthurm1
Copy link
Contributor Author

Does that mean that for android project, we need a different way to fetch the information of the source set, but the properties of the source set are the same?

I think so.

@jdneo
Copy link
Member

jdneo commented Dec 17, 2023

@Arthurm1 I have got some ideas about the refactoring. I'll make a draft these days and let you review it.

@jdneo
Copy link
Member

jdneo commented Mar 20, 2024

@Arthurm1, Sorry bro for the long time hanging. Would you like to continue the work based on the latest code base?

@Arthurm1
Copy link
Contributor Author

@jdneo In the first commit, I've refactored for the new multi-language setup. The only thing of note is I've moved compileClasspath back into GradleSourceSet as it seems to be common across all languages.

In the second commit, I've refactored the extension conversion. I've managed to make it so the conversion to a JavaExtension or ScalaExtension is done once in the DefaultGradleSourceSet constructor. It still uses reflection sadly.

The only requirement after that is the casting of Object to JavaExtension or ScalaExtension. I've pushed this into a new SupportedLanguage interface so that it's not possible to mismatch language and extension like Conversions.toJavaExtension(gradleSourceSet.getExtensions().get(SupportedLanguages.SCALA));

See what you think

@Arthurm1
Copy link
Contributor Author

@jdneo If you're happy with this refactoring then I'll refactor the Kotlin PR

Copy link
Member

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

LGTM. This PR also nicely addresses the issue(#128), which is very great!

Thank you @Arthurm1 for your contribution!

@jdneo jdneo merged commit 39c3a84 into microsoft:develop Mar 31, 2024
4 checks passed
@Arthurm1 Arthurm1 deleted the scala branch March 31, 2024 23:35
@jdneo jdneo added this to the 0.2.0 milestone Jun 12, 2024
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

Successfully merging this pull request may close these issues.

None yet

2 participants