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

[Xamarin.Android.Build.Tasks] Add @(AndroidGradleProject) action #9270

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

pjcollins
Copy link
Member

@pjcollins pjcollins commented Aug 30, 2024

Context: https://github.com/CommunityToolkit/Maui.NativeLibraryInterop

Introduces an @(AndroidGradleProject) build action which can be used
to build and consume the outputs of Android Gradle projects.

The following metadata is supported on this item:

<AndroidGradleProject Include="path/to/projectdir">
  <Bind></Bind>
  <Configuration>Release</Configuration>
  <ModuleName></ModuleName>
  <OutputPath>$(IntermediateOutputPath)gradle/{ModuleName}-{Hash}</OutputPath>
  <Pack></Pack>
  <CreateAndroidLibrary>true</CreateAndroidLibrary>
</AndroidGradleProject>

The ItemSpec or Include attribute should be the path to the root
build.gradle file of the project, next to gradlew wrapper scripts.

If the @(AndroidGradleProject) declares a library module that produces
an AAR, that AAR will be added as an @(AndroidLibrary) item
automatically.

The Bind, Pack, and Visible metadata will be applied to the
@(AndroidLibrary) item if it is created, using the defaults outlined
above.

A new $(AndroidPrepareForBuildDependsOn) build extension point has
been added to allow customer projects to more easily hook into the
beginning of the build process.

@pjcollins pjcollins added the do-not-merge PR should not be merged. label Sep 5, 2024
@pjcollins pjcollins marked this pull request as ready for review September 5, 2024 02:30
@pjcollins
Copy link
Member Author

Opening this up for some initial feedback, though I am still working on issues related to dotnet pack.

<_GradleInputs Include="%(AndroidGradleProjectReference.FullPath)/**/*.kts" />
<_GradleInputs Include="%(AndroidGradleProjectReference.FullPath)/**/*.gradle" />
<_GradleInputs Include="%(AndroidGradleProjectReference.FullPath)/**/*.properties"/>
<_GradleInputs Include="%(AndroidGradleProjectReference.FullPath)/**/*.xml" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this include the gradle equivelents of obj in the search ? becuase gradle does produce allot of intermediate files, it might cause some slow evaluations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only if the OutputPath metadata is set by the customer to a path under the Gradle project. The default build path is in the output path of the .NET project, but I think I can update this to exclude content in %(AndroidGradleProjectReference.OutputPath) in case it's overridden.

Copy link
Member

Choose a reason for hiding this comment

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

All the wildcards seem like they could be slow like I saw in:

If you have a .binlog, I wonder how much time it takes as it would run these on every build.

The current code looks like it would run ** in that folder 6 times. Is there a way it could be nonrecursive, or have less wildcards?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this to do the following instead, but still need to do some perf measuring:

<_AllGradleFiles Include="$([System.IO.Directory]::GetFiles('%(AndroidGradleProjectReference.FullPath)', '*.*', System.IO.SearchOption.AllDirectories))"
    Condition= " Exists('%(AndroidGradleProjectReference.FullPath)') "/>
<_GradleInputs Include="@(_AllGradleFiles)"
    Condition="  '%(Extension)' == '.java'
              or '%(Extension)' == '.kt'
              or '%(Extension)' == '.kts'
              or '%(Extension)' == '.gradle'
              or '%(Extension)' == '.properties'
              or '%(Extension)' == '.xml' " />

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this again to use a single wildcard glob which is then filtered. The initial glob is conditional based on the existence of the directory being searched so we don't accidentally or arbitrarily scan a bunch of directories.

Copy link
Contributor

@dellis1972 dellis1972 left a comment

Choose a reason for hiding this comment

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

Looks nice 😄 It might be worth doing a blog post for this to show people how to use it 😄

@jpobst
Copy link
Contributor

jpobst commented Sep 5, 2024

How does this handle dependencies? Let's say my gradle project depends on org.jetbrains.kotlin:kotlin-stdlib:2.0.0.

Is this:

  • Dependency is left entirely up to the user, in this case they would need to add our NuGet package to their project.
  • Native dependency is automatically included, which would conflict with any usage of our NuGet package.
  • Something else?

@pjcollins
Copy link
Member Author

How does this handle dependencies? Let's say my gradle project depends on org.jetbrains.kotlin:kotlin-stdlib:2.0.0.

Is this:

  • Dependency is left entirely up to the user, in this case they would need to add our NuGet package to their project.
  • Native dependency is automatically included, which would conflict with any usage of our NuGet package.
  • Something else?

There is currently no dependency management functionality, it's left up to the user to figure out what is needed and what is best. This is a scenario I am hoping we can try to improve in the future, perhaps through recommending some combination of NuGet or AndroidMavenLibrary references. There are also likely different use cases for application vs library projects where we could potentially try to do some automatic dependency inclusion in the former case.

@pjcollins
Copy link
Member Author

Looks nice 😄 It might be worth doing a blog post for this to show people how to use it 😄

We have a "v1" of sorts out in https://github.com/CommunityToolkit/Maui.NativeLibraryInterop and https://learn.microsoft.com/en-us/dotnet/communitytoolkit/maui/native-library-interop , and will be planning to update these with this functionality when it's available

<_GradleInputs Include="%(AndroidGradleProjectReference.FullPath)/**/*.kts" />
<_GradleInputs Include="%(AndroidGradleProjectReference.FullPath)/**/*.gradle" />
<_GradleInputs Include="%(AndroidGradleProjectReference.FullPath)/**/*.properties"/>
<_GradleInputs Include="%(AndroidGradleProjectReference.FullPath)/**/*.xml" />
Copy link
Member

Choose a reason for hiding this comment

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

All the wildcards seem like they could be slow like I saw in:

If you have a .binlog, I wonder how much time it takes as it would run these on every build.

The current code looks like it would run ** in that folder 6 times. Is there a way it could be nonrecursive, or have less wildcards?

Comment on lines 51 to 61
<!-- Run assemble task for project outputs, android app and library project outputs are currently supported -->
<Gradle ToolPath="%(AndroidGradleProjectReference.FullPath)"
Command="assemble%(AndroidGradleProjectReference.Configuration)"
ModuleName="%(AndroidGradleProjectReference.ModuleName)"
OutputPath="%(AndroidGradleProjectReference.OutputPath)"
IntermediateOutputPath="$(IntermediateOutputPath)"
AndroidSdkDirectory="$(AndroidSdkDirectory)"
JavaSdkDirectory="$(JavaSdkDirectory)" >
<Output TaskParameter="AppOutputs" ItemName="AndroidGradleProjectAppOutputs" />
<Output TaskParameter="LibraryOutputs" ItemName="AndroidGradleProjectLibraryOutputs" />
</Gradle>
Copy link
Member

Choose a reason for hiding this comment

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

One thought is if we can build these in parallel. Batching like %(AndroidGradleProjectReference) would just run it N times one after another. We can think about this in a future PR.

src/Xamarin.Android.Build.Tasks/Tasks/Gradle.cs Outdated Show resolved Hide resolved
src/Xamarin.Android.Build.Tasks/Tasks/Gradle.cs Outdated Show resolved Hide resolved
@jpobst
Copy link
Contributor

jpobst commented Sep 9, 2024

There is currently no dependency management functionality, it's left up to the user to figure out what is needed and what is best. This is a scenario I am hoping we can try to improve in the future, perhaps through recommending some combination of NuGet or AndroidMavenLibrary references.

If a .pom file is produced by the gradle project, adding it to the AndroidLibrary with the Manifest attribute will opt-into our Java dependency verification which would provide dependency enforcement and NuGet package suggestions:

https://review.learn.microsoft.com/en-us/dotnet/android/binding-libs/advanced-concepts/java-dependency-verification?branch=binding-docs

@pjcollins
Copy link
Member Author

There is currently no dependency management functionality, it's left up to the user to figure out what is needed and what is best. This is a scenario I am hoping we can try to improve in the future, perhaps through recommending some combination of NuGet or AndroidMavenLibrary references.

If a .pom file is produced by the gradle project, adding it to the AndroidLibrary with the Manifest attribute will opt-into our Java dependency verification which would provide dependency enforcement and NuGet package suggestions:

https://review.learn.microsoft.com/en-us/dotnet/android/binding-libs/advanced-concepts/java-dependency-verification?branch=binding-docs

Excellent, I will try to look into this after the initial work lands.

@pjcollins pjcollins removed the do-not-merge PR should not be merged. label Sep 9, 2024
cmd.AppendSwitch (Arguments);

// Do not leave gradle daemon running
cmd.AppendSwitch ("--no-daemon");
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this is that if a project contains multiple @(AndroidGradleProjectReference)s, each build will take longer.

A "better" / alternative idea may be to have the <Gradle/> task take all of the projects, and build them in parallel:

<Gradle Projects="@(AndroidGradleProject) … />

and then use have Gradle inherit from AsyncTask a'la <MavenDownload/>/<Aapt/>/etc., execute gradlew from all of the projects in parallel, and then at the end finish with gradlew --stop.

A problem with this idea is that it assumes that each gradlew is "the same", which might not be true. (…and then what? Run gradlew --stop for each project separately?)

This also assumes that gradlew is located alongside each build.gradle separately. That might be true for some customers, but that certainly isn't true for us; look at git grep GradleWPath, and see that it's used from three separate projects, so that we have only one gradlew in the repo, not N.

Copy link
Member Author

Choose a reason for hiding this comment

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

The "safe" approach for now in my mind would be to keep builds running in sequence in isolated processes. I think we can look at a performance pass in a future PR as Peppers also mentioned, where we can support building in parallel and/or potentially re-using the daemon as well.

@pjcollins pjcollins changed the title [Xamarin.Android.Build.Tasks] Add @(AndroidGradleProjectReference) [Xamarin.Android.Build.Tasks] Add @(AndroidGradleProject) action Sep 11, 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.

5 participants