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] fix - leverage build tree path #167

Closed
wants to merge 1 commit into from
Closed

Conversation

jdneo
Copy link
Member

@jdneo jdneo commented Jul 9, 2024

@Arthurm1 Would like to hear your input on this change proposal.

Background

Previously, the build server implementation does not handle the include build well, because we didn't know the 'trick' of build action. So the project dependencies were not recovered from the build tree root.

To fix some of the include build bugs, #107 was introduced. But I found that #107 has its own problem:

  • sourceSet.getCompileClasspath().getFiles() may return runtime exceptions, and thus cause the import failure.
  • Some internal apis are used when we tried to establish the relationship between the output jars and the source sets.

Now, since we can use build action, it seems that the solution can be simpler.

Considering that, in the BuildTargetManager, only main sourceset is considered as the build target dependencies, looks like the old implementation is enough to handle the project dependencies.

I've two proposals on this:

  1. Which is this PR, it marks the changes in handle different project dependency configs #107 as an experimental feature, since handle different project dependency configs #107 can give more accurate dependency relationship between source sets.
  2. Revert the change in handle different project dependency configs #107, and related types, like GradleSourceSetsMetadata, which may makes the code base more simpler.

WDYT?

@Arthurm1
Copy link
Contributor

Arthurm1 commented Jul 9, 2024

If there are issues and you want to put them behind an experimental setting then I think that's fine.

Some things I don't understand...

  1. I'm not sure how sourceSet.getCompileClasspath().getFiles() would produce an exception?
  2. Does Project#getBuildTreePath exist pre 8.3? What does it return instead of Project#getPath? Is it the full path? If so - doesn't the project name in BSP look very long now?

Now that BuildActions are being used, I'd like to see the SourceSetsModelBuilder changed to retrieve a single project and then the inter-project dependencies could be updated in the build action. This would allow the source sets to be evaluated concurrently which would speed up Scala sourceset retrieval as scalaCompile#getScalaClasspath#getFiles is slow and that adds up with multiple projects.
I'd also like to get rid of the "main".equals(sourceSet.getSourceSetName()) to allow whatever sourcesets users have defined have inter-dependencies.

I'm happy to submit a PR to allow sourceset retrieval in parallel. If you wanted to merge your changes here then I'll wait for that.

@jdneo
Copy link
Member Author

jdneo commented Jul 9, 2024

I'm not sure how sourceSet.getCompileClasspath().getFiles() would produce an exception?

Usually this caused by user errors I think. For example, when user declares a wrong dependency. To test it, you can simply using a gradle project which has dependencies, but does not declare repositories:

repositories {
	// mavenCentral()  <-- no repo
}

Does Project#getBuildTreePath exist pre 8.3

Ah, my bad, no it does not exist when <8.3 😢

What does it return instead of Project#getPath? Is it the full path?

Yes it's full build path.

This would allow the source sets to be evaluated concurrently which would speed up Scala sourceset retrieval as scalaCompile#getScalaClasspath#getFiles is slow and that adds up with multiple projects.

Will does boost the overall performance? Or it's for scala only?

@Arthurm1
Copy link
Contributor

Arthurm1 commented Jul 9, 2024

Ah, my bad, no it does not exist when <8.3 😢

It might work. Some methods require >= 8.3 tooling API but work for all versions of Gradle. I'm not sure how to work this out though.

@jdneo
Copy link
Member Author

jdneo commented Aug 13, 2024

Close as it's not required for now.

In the future, if we want to revisit this, we can try to cast the object to DefaultXXX and call the method from it for the build tree path.

@jdneo jdneo closed this Aug 13, 2024
@jdneo jdneo deleted the cs/build-tree-path branch August 13, 2024 09:09
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.

2 participants