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

Composite Builds using Build Actions #154

Merged
merged 11 commits into from
Jul 5, 2024

Conversation

Tanish-Ranjan
Copy link
Contributor

This PR continues the work done by @Arthurm1 for composite-builds on #122, bringing in support for included builds, including dependency substitution using build actions.

@Tanish-Ranjan Tanish-Ranjan marked this pull request as ready for review June 21, 2024 15:27
@Tanish-Ranjan
Copy link
Contributor Author

@microsoft-github-policy-service agree

@Arthurm1
Copy link
Contributor

@Tanish-Ranjan Thanks for the PR.

I see you've used a Gradle BuildAction. Was that so you don't have to query the source sets from GradleAPIConnector for each included build?

If that's the best route then you probably don't need the GradleIncludedBuild class as the model never returns any.

I think you'll still need to work out the project dependencies across included builds. Maybe you can put that in the BuildAction as well. I've slightly altered your new test to show what I mean. This dependency check currently fails...

  @Test
  void testCompositeBuild2() {
    File projectDir = projectPath.resolve("composite-build-2").toFile();
    PreferenceManager preferenceManager = new PreferenceManager();
    preferenceManager.setPreferences(new Preferences());
    GradleApiConnector connector = new GradleApiConnector(preferenceManager);
    GradleSourceSets gradleSourceSets = connector.getGradleSourceSets(projectDir.toURI(), null);
    assertEquals(6, gradleSourceSets.getGradleSourceSets().size());
    GradleSourceSet mainApp = findSourceSet(gradleSourceSets, "app [main]");
    GradleSourceSet mainStringUtils = findSourceSet(gradleSourceSets, "string-utils [main]");
    GradleSourceSet mainNumberUtils = findSourceSet(gradleSourceSets, "number-utils [test]");
    assertHasBuildTargetDependency(mainApp, mainStringUtils);
    assertHasBuildTargetDependency(mainApp, mainNumberUtils);
  }

@Tanish-Ranjan
Copy link
Contributor Author

I see you've used a Gradle BuildAction. Was that so you don't have to query the source sets from GradleAPIConnector for each included build?

That's correct @Arthurm1. Gradle automatically manages substitution of dependencies. It didn't work previously in commit f2097d0 because Gradle was getting scoped to the project, loosing reference of the overall build and hence was not able to perform dependency substitutions from the included builds. With the help of Gradle TAPI's BuildAction we are maintaining the build scope while we fetch the GradleSourceSets.

@Tanish-Ranjan
Copy link
Contributor Author

I think you'll still need to work out the project dependencies across included builds.

Thanks for pointing it out. I'll look into it.

@jdneo jdneo self-requested a review June 24, 2024 01:57
Copy link

@reinsch82 reinsch82 left a comment

Choose a reason for hiding this comment

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

This looks good to me.
Just a few remarks.
I think you have to wait for @jdneo's review though.

Arthur McGibbon and others added 6 commits July 3, 2024 10:16
This commit updates this branch to reflect the latest changes in the main branch. This ensures the branch stays aligned with the current development state.
This commit addresses checkstyle warnings identified in the GradleApiConnector.java
This commit refactors the CompositeBuild tests for better organization and clarity.
Changes:
- Moved the existing composite-build test to a new test named "composite-build-1"
- Added a new test named "composite-build-2" to verify dependency substitution functionality.
This commit improves the code by delegating the logic for retrieving source sets to a dedicated build action named `GetSourceSetsAction`. This enhances code organization and separation of concerns.

Additionally, the test case `testCompositeBuild2` has been updated to verify if the new approach correctly retrieves the expected source sets.
Changes:
- Removed unused files: DefaultGradleIncludedBuild.java and GradleIncludedBuild.java
- Removed leftover implementations from DefaultGradleSourceSets.java
- Moved dependency mapping logic from SourceSetsModelBuilder.java to GetSourceSetsBuildAction.java
- Introduced GradleSourceSetsMetadata.java for passing information between SourceSetsModelBuilder and GetSourceSetsBuildAction for dependency mapping
- Updated GradleApiConnectorTest.java#testCompositeBuild2 to verify proper dependency mapping
- Updated GradleBuildServerPluginTest.java#getGradleSourceSets to receive GradleSourceSetsMetadata and return expected GradleSourceSets
@jdneo jdneo added the bug Something isn't working label Jul 3, 2024
@jdneo jdneo added this to the 0.3.0 milestone Jul 3, 2024
Changes:
- Added missing import statements for improved readability.
- Utilized Java 17 features for concise code (var type inference).
- Addressed checkstyle warnings to maintain code consistency.
Changes:
- Refactored variable and method names to reflect their purpose clearly.
- Updated JavaDoc comments for better explanation.
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.

Tested with Gradle official composite build sample and a composite build project that provided by a user. Both works as expected.

Thank you @Tanish-Ranjan and @Arthurm1 for the contribution, good job!

@jdneo jdneo merged commit 5af4df6 into microsoft:develop Jul 5, 2024
4 checks passed
@jdneo jdneo mentioned this pull request Jul 5, 2024
@Tanish-Ranjan
Copy link
Contributor Author

Big thanks to @Arthurm1 for the initial insights into the tooling API! Your guidance gave this PR a significant head start. Additionally, a huge thanks to all the gsoc mentors (@jdneo @donat @hegyibalint @reinsch82) who provided support throughout this process. Your help was invaluable in getting this completed so quickly.

@Tanish-Ranjan Tanish-Ranjan deleted the composite-builds branch July 5, 2024 07:25
@jdneo
Copy link
Member

jdneo commented Jul 10, 2024

Ok, so after some investigation, I found that the current solution has some problems:

  1. See The compile of build action #169 and fix - Unsupported class file for the build action class #170
  2. For the sample project provided in Can't resolve imports when using includeBuild in package with v3.13.0 vscode-gradle#1435, all the sourceset from two subproject will have the same project path :, this is actually not correct, we need to use the build tree path to differentiate them. Otherwise, the build target dependency resolution in BuildTargetManager might be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants