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] remove Xamarin.Android.Windows.targets #2561

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Dec 20, 2018

Fixes: http://work.devdiv.io/707557

In 539954c, I was aiming to remove the
_RegisterAndroidFilesWithFileWrites MSBuild target (runs only on
Windows). I was having trouble getting everything to work, so I undid
this change to make the PR smaller.

Reasons to remove _RegisterAndroidFilesWithFileWrites:

  • It looks like it may be a source of incremental build bugs. It
    causes $(CleanFile) to grow on incremental builds indefinitely...
  • We should instead use the FileWrites item group appropriately.
  • We can avoid ~81ms of build time.
81 ms  _RegisterAndroidFilesWithFileWrites        1 calls

Revisiting it now, it looks like we can remove
Xamarin.Android.Windows.targets completely!

  • _RegisterAndroidFilesWithFileWrites is the only target.
  • The $(_IsRunningXBuild) property is only used in this file.
  • I moved the $(Debugger) property to
    Xamarin.Android.Common.props. I believe this value is only used in
    VS Windows, but it make more sense to be here.

CopyGeneratedJavaResourceClasses

The <CopyGeneratedJavaResourceClasses/> MSBuild task was copying an
R.java file from %TEMP% to:

$(IntermediateOutputPath)android\unnamedproject\unnamedproject\R.java

We only use this file in one place, since the rest of the build uses:

$(IntermediateOutputPath)android\src\unnamedproject\unnamedproject\R.java

We can avoid copying this extra file and adding it to FileWrites:

  • <CopyGeneratedJavaResourceClasses/> now optionally uses the
    DestinationTopDirectory property
  • If DestinationTopDirectory is blank, the original source file is
    returned as the PrimaryJavaResgenFile output property.

We now use the R.java from %TEMP% directly, and avoid copying a
file to $(IntermediateOutputPath).

Other Changes

I modified the CheckNothingIsDeletedByIncrementalClean test to
verify that $(CleanFile) isn't growing on incremental builds.

Anything added to FileWrites during the _CompileDex MSBuild target
(or its dependent targets) wasn't showing up in $(CleanFile)! I
adjusted its BeforeTargets to match what I did in 539954c for
_PrepareAssemblies.

Two files needed to be added to FileWrites:

  • $(_PackagedResources)
  • $(_GeneratedPrimaryJavaResgenFile)

@jonathanpeppers jonathanpeppers added the do-not-merge PR should not be merged. label Dec 20, 2018
@jonathanpeppers
Copy link
Member Author

Need to look into test failures.

@jonathanpeppers
Copy link
Member Author

@dellis1972 I need to ask you about this after the break: https://github.com/xamarin/xamarin-android/blob/46f02511f8c91b133fb09fdcad2fa8fe9f2cc730/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L1795-L1801

This task creates $(IntermediateOutputPath)android\unnamedproject\unnamedproject\R.java, but I normally expect this file in $(IntermediateOutputPath)android\src\unnamedproject\unnamedproject\R.java.

Is this path wrong, or are we deliberately keeping two copies? I'm seeing the one not in src getting deleted by IncrementalClean.

@dellis1972
Copy link
Contributor

@jonathanpeppers we need to figure out where that $(IntermediateOutputPath)android\unnamedproject\unnamedproject\R.java is being used.. If its a .java file it should be in src afaik.

@jonathanpeppers
Copy link
Member Author

@dellis1972 it looks like the only thing using this file: $(IntermediateOutputPath)android\unnamedproject\unnamedproject\R.java

Is the <GenerateResourceDesigner/> task right below it using $(_GeneratedPrimaryJavaResgenFile):
https://github.com/xamarin/xamarin-android/blob/d1a307097419aa80608ee3636bc9f30aa6860f75/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L1263-L1277

It looks like:

  • <Aapt/> puts R.java in %TEMP%
  • <CopyGeneratedJavaResourceClasses/> finds it in %TEMP% and copies to $(IntermediateOutputPath)android\unnamedproject\unnamedproject\R.java
  • <GenerateResourceDesigner/> uses $(IntermediateOutputPath)android\unnamedproject\unnamedproject\R.java
  • <RemoveDirFixed/> deletes the temp dir

I'm thinking I could just rework this to use the R.java directly from %TEMP%? And not copy anything?

@jonathanpeppers jonathanpeppers force-pushed the remove-xamarin.android.windows.targets branch from 3fcb8c9 to 82fd227 Compare January 2, 2019 17:03
@jonathanpeppers jonathanpeppers removed the do-not-merge PR should not be merged. label Jan 2, 2019
Fixes: http://work.devdiv.io/707557

In 539954c, I was aiming to remove the
`_RegisterAndroidFilesWithFileWrites` MSBuild target (runs only on
Windows). I was having trouble getting everything to work, so I undid
this change to make the PR smaller.

Reasons to remove `_RegisterAndroidFilesWithFileWrites`:

- It looks like it *may* be a source of incremental build bugs. It
  causes `$(CleanFile)` to grow on incremental builds indefinitely...
- We should instead use the `FileWrites` item group appropriately.
- We can avoid ~81ms of build time.

    81 ms  _RegisterAndroidFilesWithFileWrites        1 calls

Revisiting it now, it looks like we can remove
`Xamarin.Android.Windows.targets` completely!

- `_RegisterAndroidFilesWithFileWrites` is the only target.
- The `$(_IsRunningXBuild)` property is only used in this file.
- I moved the `$(Debugger)` property to
  `Xamarin.Android.Common.props`. I believe this value is only used in
  VS Windows, but it make more sense to be here.

~~ CopyGeneratedJavaResourceClasses ~~

The `<CopyGeneratedJavaResourceClasses/>` MSBuild task was copying an
`R.java` file from `%TEMP%` to:

    $(IntermediateOutputPath)android\unnamedproject\unnamedproject\R.java

We only use this file in one place, since the rest of the build uses:

    $(IntermediateOutputPath)android\src\unnamedproject\unnamedproject\R.java

We can avoid copying this extra file and adding it to `FileWrites`:

- `<CopyGeneratedJavaResourceClasses/>` now optionally uses the
  `DestinationTopDirectory` property
- If `DestinationTopDirectory` is blank, the original source file is
  returned as the `PrimaryJavaResgenFile` output property.

We now use the `R.java` from `%TEMP%` directly, and avoid copying a
file to `$(IntermediateOutputPath)`.

~~ Other Changes ~~

I modified the `CheckNothingIsDeletedByIncrementalClean` test to
verify that `$(CleanFile)` isn't growing on incremental builds.

Anything added to `FileWrites` during the `_CompileDex` MSBuild target
(or its dependent targets) wasn't showing up in `$(CleanFile)`! I
adjusted its `BeforeTargets` to match what I did in 539954c for
`_PrepareAssemblies`.

Two files needed to be added to `FileWrites`:

- `$(_PackagedResources)`
- `$(_GeneratedPrimaryJavaResgenFile)`
`obj/Debug/android/**/R.java` no longer exists after a design-time build.

We can still look for `obj/Debug/android/src/**/R.java` after a full build.
@jonathanpeppers jonathanpeppers force-pushed the remove-xamarin.android.windows.targets branch from 82fd227 to d7c3421 Compare January 2, 2019 20:06
@jonathanpeppers jonathanpeppers requested a review from garuma January 2, 2019 22:56
@dellis1972 dellis1972 merged commit a30dd21 into dotnet:master Jan 3, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Feb 1, 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.

3 participants