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] property refactoring/cleanup #3623

Merged
merged 1 commit into from
Sep 16, 2019

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Sep 12, 2019

Context: master...jonathanpeppers:fully-incremental

I am working on a revamp of the <GenerateJavaStubs/> MSBuild task to
make it "fully incremental".

The following parts of the build would happen on a per-assembly basis:

  • Java sources generated
  • Java class files compiled via javac
  • classes.zip or *.jar files

This will be hidden behind a feature flag such as
/p:_AndroidFullyIncremental=True for its first implementation.

The problem I am facing is that our build throws random Java sources
into $(IntermediateOutputPath)android\src throughout the build. This
includes files like R.java, MonoRuntimeProvider.java, etc.

To make it where the location of these files is modifiable, we can
create a few new private properties:

  • $(_AndroidIntermediateJavaSourceDirectory) which is $(IntermediateOutputPath)android\src\
  • $(_AndroidIntermediateJavaClassDirectory) which is $(IntermediateOutputPath)android\bin\classes\
  • $(_AndroidIntermediateClassesZip) which is $(IntermediateOutputPath)android\bin\classes.zip

This way I can modify the location when the _AndroidFullyIncremental
feature flag is set.

To refactor things a bit further, I also removed usage of
$(MonoAndroidIntermediate) and just used $(IntermediateOutputPath)
instead. $(IntermediateOutputPath) should be generally more
well-known for MSBuild, and I kept the original property around
because I found usage in other repos.

@dellis1972
Copy link
Contributor

I think we have been using Path or Directory for must of our properties historically, rather than Dir, it is only a minor thing.

Also I found it seem there is a "convention" in MSBuild. Things ending with Path are relative paths, things ending with Directory are full paths. Not sure if that is official, and I'm sure we break those rules even if they are 🤷‍♂ Otherwise this looks good, it gives us options :)

  <Target Name="Foo">
    <Message Text="$(IntermediateOutputPath)" />
    <Message Text="$(MSBuildThisFileDirectory)" />
  </Target>
Foo:
  obj/Debug/
  /Users/dean/Projects/foo645539/

@jonathanpeppers
Copy link
Member Author

I was looking at the list here: https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-reserved-and-well-known-properties

There are ones one here named *Path that are full paths, like MSBuildToolsPath. Then they have others like MSBuildThisFileFullPath... Maybe no one is consistent?

I think I could just rename Dir -> Directory and that reads better.

@jonathanpeppers
Copy link
Member Author

It looks like there is a change needed on the monodroid side, let me look into that.

Context: dotnet/android@master...jonathanpeppers:fully-incremental

I am working on a revamp of the `<GenerateJavaStubs/>` MSBuild task to
make it "fully incremental".

The following parts of the build would happen on a per-assembly basis:

* Java sources generated
* Java class files compiled via `javac`
* `classes.zip` or `*.jar` files

This will be hidden behind a feature flag such as
`/p:_AndroidFullyIncremental=True` for its first implementation.

The problem I am facing is that our build throws random Java sources
into `$(IntermediateOutputPath)android\src` throughout the build. This
includes files like `R.java`, `MonoRuntimeProvider.java`, etc.

To make it where the location of these files is modifiable, we can
create a few new private properties:

* `$(_AndroidIntermediateJavaSourceDirectory)` which is `$(IntermediateOutputPath)android\src\`
* `$(_AndroidIntermediateJavaClassDirectory)` which is `$(IntermediateOutputPath)android\bin\classes\`
* `$(_AndroidIntermediateClassesZip)` which is `$(IntermediateOutputPath)android\bin\classes.zip`

This way I can modify the location when the `_AndroidFullyIncremental`
feature flag is set.

To refactor things a bit further, I also removed usage of
`$(MonoAndroidIntermediate)` and just used `$(IntermediateOutputPath)`
instead. `$(IntermediateOutputPath)` should be generally more
well-known for MSBuild, and I kept the original property around
because I found usage in other repos.
@jonathanpeppers jonathanpeppers merged commit 185c529 into dotnet:master Sep 16, 2019
@jonathanpeppers jonathanpeppers deleted the msbuild-properties branch September 16, 2019 13:47
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2024
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.

None yet

3 participants