-
Notifications
You must be signed in to change notification settings - Fork 528
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] fix incremental builds for Xamarin.Forms projects #2088
[Xamarin.Android.Build.Tasks] fix incremental builds for Xamarin.Forms projects #2088
Conversation
@dellis1972 when you're back (after I got this green), I want to check I'm doing the right thing with the two flag files passed to the two times It seemed like they were always "fighting" each other and updating the same file in a Xamarin.Forms app: in a build with no changes. |
AcwMapFile="$(_AcwMapFile)" | ||
AndroidConversionFlagFile="$(IntermediateOutputPath)_javastubs.stamp" | ||
/> | ||
<Touch Files="$(IntermediateOutputPath)_javastubs.stamp;$(_AndroidResgenFlagFile)" AlwaysCreate="True" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the part I was worried about, will touching $(_AndroidResgenFlagFile)
have any ill effects?
This is what made this PR green, and prevented the two ConvertResourcesCases
from fighting each other.
@jonathanpeppers we need to be careful around this area.
So if GenerateJavaStubs runs we currently need to then process ALL the resources again so fix up those views. That needs some thinking since I would love to figure out a way of not having to process ALL the files again.. just those which contains stuff that looks like custom views. To be honest I was gonna put in a Touch for the |
ManifestPlaceholders="$(AndroidManifestPlaceholders)" | ||
OutputDirectory="$(IntermediateOutputPath)android" | ||
MergedAndroidManifestOutput="$(IntermediateOutputPath)android\AndroidManifest.xml" | ||
ManifestTemplate="$(_AndroidManifestAbs)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonpryor is not gonna like all the space changes here. The rule is we need to be consistent with the existing code in that part of the file. This is until we can split this file up into smaller chunks and reformat :)
Noob question: So after fixing this issue, the situation described above now takes ~0s instead of xx seconds? Build |
@pierceboggan on @StephaneDelcroix's machine:
NOTE: this was a Debug/master build of Xamarin.Android These were running even if your build had no changes. These won't run anymore if you have no changes. It looked to me this file that is in the Xamarin.Forms template causes it: <android.support.design.widget.TabLayout
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
android:id="@+id/sliding_tabs"
android:background="?attr/colorPrimary\"
android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
app:tabIndicatorColor="@android:color/white"
app:tabGravity="fill" app:tabMode="fixed" /> It wasn't happening in a "Hello World" app, or the existing tests in Xamarin.Android. |
ae72d08
to
1f862e8
Compare
Looking into these new test failures on Mac... It was green, then all I did was rebase + undo formatting changes? |
@jonpryor this is an example of the test failures grendel was talking about:
|
@jonathanpeppers: wrt your previous comment, mono apparently doesn't reliably like (or dislike) whatever our unit tests are doing: Sometimes they cause mono to crash. I'm not sure if these are mono bugs or Java.Interop bugs; they require investigation. |
…s projects @StephaneDelcroix noticed a build performance problem in Xamarin.Forms projects: - Build - Build again with no changes - Slow targets such as `_UpdateAndroidResgen` and `_GenerateJavaStubs` run again! The cause appeared to be this: Building target "_UpdateAndroidResgen" completely. Input file "obj/Debug/res/layout/tabbar.xml" is newer than output file "obj/Debug/R.cs.flag". I modified the `CheckTimestamps` test to replicate this issue: - Added Xamarin.Forms NuGet packages - Added `Tabbar.axml` from the Xamarin.Forms template This surfaced several problems with timestamps and Xamarin.Android building incrementally... ~~ Problem 1 ~~ In the `_GenerateJavaStubs` target, the `ConvertResourcesCases` MSBuild task is not supplied a `AndroidConversionFlagFile`. It uses this file to compare timestamps and decide if XML files need fixed up or not. Supplying `$(_AndroidResgenFlagFile)` seemed to be the way to go here, since `_UpdateAndroidResgen` runs right before this task. It also solves the original problem from the `Tabbar.axml` file. I also realized we should be using a "stamp" file for the `_GenerateJavaStubs` MSBuild target, in general. This gives us several benefits: - `ConvertResourcesCases` has a proper `AndroidConversionFlagFile` it can use for checking timestamps - The `Outputs` of the target are drastically simplified, this should help peformance in MSBuild evaluating if the target should run - It is likely more precise/correct. So I added a new file in `$(IntermediateOutputPath)`, `obj\Debug\_javastubs.stamp` we use to determine if `_GenerateJavaStubs` should run. I also tried to do things "the right way", such as: - Adding the stamp file to the `FileWrites` item group - Made sure the file is deleted during a `Clean` After these changes, two instances of `ConvertResourcesCases` were *still* fighting each other. They would always run and update the timestamps on one file: causing the other one to run. `ConvertResourcesCases` runs twice: - In the `_UpdateAndroidResgen` target - In the `_GenerateJavaStubs` target But these are both working with the same files, so in the second case, we need to make sure `_GenerateJavaStubs` updates the timestamp on the *first* flag file so the target doesn't run every time. ~~ Problem 2 ~~ Timestamps of Xamarin.Forms assemblies in `obj\Debug\linksrc` were out of date! I updated the `_CopyIntermediateAssemblies` target to update the timestamps of all files within this directory. It did not appear to be doing it correctly, and was only running `<Touch />` on a subset of assemblies. ~~ Problem 3 ~~ After fixing No. 2, the next problem I noticed were `*.pdb` and `*.mdb` files in `obj\Debug\linksrc` were out of date! The problem here was a call to `<Touch />`: <Touch Files="@(_DebugFilesCopiedToLinkerSrc)" /> The `_DebugFilesCopiedToLinkerSrc` did not appear to contain any items! I updated the `_CopyPdbFiles` and `_CopyMdbFiles` targets to use the correct item groups. ~~ Problem 4 ~~ Lastly, after fixing the previous three issues, the timestamps of output assemblies in `obj\Release\android\assets` were not correct for `Release` builds. I had made a change for this in the past, which was making `Debug` builds work correctly: dotnet#2028 At the time I wrote dotnet#2028, since the test wasn't using Xamarin.Forms, I did not see the problem ocurring in `Release` mode. It *does* occur if you are using NuGet packages. I replicated what I changed in the `_LinkAssembliesNoShrink` target, and also renamed the item groups to be unique: - `_LinkAssembliesNoShrinkFiles` - `_LinkAssembliesShrinkFiles` ~~ Other changes ~~ The `CheckTimestamps` test also was verifying timestamps in `$(OutputPath)` such as `bin\Debug`. It appears the core MSBuild targets are leaving timestamps as-is in the `CopyFilesToOutputDirectory` and `_CopyFilesMarkedCopyLocal` targets. Assemblies from Xamarin.Forms NuGet are out of date here, but I don't think we should do anything, since the core MSBuild targets are doing this. For now, I removed the checks for `$(OutputPath)` in this test, and cleaned up the test a bit.
This fixes the failing test on MacOS. It appears I'm seeing diffent behavior running on Mono, and I'm having to explicitly set the timestamps on files being extracted from a zip. I also couldn't use `MonoAndroidHelper`, since it's method that does this requires the MSBuild logger.
c6090ca
to
1ba55d7
Compare
Rebasing and hoping we can get a green build here. |
…Changed Context: dotnet#2088 970da9e was a good step towards "correctness" in building incrementally in the following scenario: - File | New Xamarin.Forms project | NetStandard library - Build - Change XAML - Build In this scenario, there is now a new target rising to the surface we can improve: 276 ms _CopyIntermediateAssemblies 1 calls Looking at the target, it seems we could use the `CopyIfChanged` task here more effectively. This task will automaticaly set the timestamps of files that have been copied, and so we don't need any subsequent `<ItemGroup />` or `<Touch />` elements. It was also touching *all* files instead of just the ones that were changed. After this change: 33 ms _CopyIntermediateAssemblies 1 calls The overall build went from 7.058s to 6.652s, so there must be some other targets that benefit from the timestamps not changing on *all* of these files.
…Changed (#2128) Context: #2088 970da9e was a good step towards "correctness" in building incrementally in the following scenario: - File | New Xamarin.Forms project | NetStandard library - Build - Change XAML - Build In this scenario, there is now a new target rising to the surface we can improve: 276 ms _CopyIntermediateAssemblies 1 calls Looking at the target, it seems we could use the `CopyIfChanged` task here more effectively. This task will automaticaly set the timestamps of files that have been copied, and so we don't need any subsequent `<ItemGroup />` or `<Touch />` elements. It was also touching *all* files instead of just the ones that were changed. After this change: 33 ms _CopyIntermediateAssemblies 1 calls The overall build went from 7.058s to 6.652s, so there must be some other targets that benefit from the timestamps not changing on *all* of these files.
Looks like this change affects the runtime performance, https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/plot/Tests%20times/ - check the XForms sample times. Do you know what might have caused that? |
…s projects (#2088) @StephaneDelcroix noticed a build performance problem when building Xamarin.Forms projects: - Build - Build again with no changes - Slow targets such as `_UpdateAndroidResgen` and `_GenerateJavaStubs` run again! The cause appeared to be this: Building target "_UpdateAndroidResgen" completely. Input file "obj/Debug/res/layout/tabbar.xml" is newer than output file "obj/Debug/R.cs.flag". I modified the `CheckTimestamps()` test to replicate this issue: - Added Xamarin.Forms NuGet packages - Added `Tabbar.axml` from the Xamarin.Forms template This surfaced several problems with timestamps and Xamarin.Android building incrementally. ~~ Problem 1 ~~ In the `_GenerateJavaStubs` target, the `<ConvertResourcesCases/>` MSBuild task is not supplied an `AndroidConversionFlagFile`. It uses this file to compare timestamps and decide if XML files need to be fixed up or not. Supplying `$(_AndroidResgenFlagFile)` seemed to be the way to go here, since `_UpdateAndroidResgen` runs right before this task. It also solves the original problem from the `Tabbar.axml` file. I also realized we should be using a "stamp" file for the `_GenerateJavaStubs` MSBuild target, in general. This gives us several benefits: - `<ConvertResourcesCases/>` has a proper `AndroidConversionFlagFile` it can use for checking timestamps - The `Outputs` of the target are drastically simplified, which may help performance in MSBuild evaluating if the target should run - It is likely more precise/correct. So I added a new file in `$(IntermediateOutputPath)`, `obj\Debug\_javastubs.stamp` we use to determine if `_GenerateJavaStubs` should run. I also tried to do things "the right way", such as: - Adding the stamp file to the `@(FileWrites)` item group - Made sure the file is deleted during a `Clean` After these changes, two instances of `<ConvertResourcesCases/>` were *still* fighting each other. They would always run and update the timestamps on one file, causing the other one to run. `ConvertResourcesCases` runs twice: - In the `_UpdateAndroidResgen` target - In the `_GenerateJavaStubs` target But these are both working with the same files, so in the second case, we need to make sure `_GenerateJavaStubs` updates the timestamp on the *first* flag file so the target doesn't run every time. ~~ Problem 2 ~~ Timestamps of Xamarin.Forms assemblies in `obj\Debug\linksrc` were out of date! I updated the `_CopyIntermediateAssemblies` target to update the timestamps of all files within this directory. It did not appear to be doing it correctly, and was only running `<Touch/>` on a subset of assemblies. ~~ Problem 3 ~~ After fixing Problem 2, the next problem I noticed were `*.pdb` and `*.mdb` files in `obj\Debug\linksrc` were out of date! The problem here was a call to `<Touch />`: <Touch Files="@(_DebugFilesCopiedToLinkerSrc)" /> The `@(_DebugFilesCopiedToLinkerSrc)` did not appear to contain any items! I updated the `_CopyPdbFiles` and `_CopyMdbFiles` targets to use the correct item groups. ~~ Problem 4 ~~ Lastly, after fixing the previous three problems, the timestamps of output assemblies in `obj\Release\android\assets` were not correct for `Release` builds. I had made a change for this in the past, which was making `Debug` builds work correctly: #2028 At the time I wrote #2028, since the test wasn't using Xamarin.Forms, I did not see the problem occurring in `Release` mode. It *does* occur if you are using NuGet packages. I replicated what I changed in the `_LinkAssembliesNoShrink` target, and also renamed the item groups to be unique: - `_LinkAssembliesNoShrinkFiles` - `_LinkAssembliesShrinkFiles` ~~ Other changes ~~ The `CheckTimestamps()` test also was verifying timestamps in `$(OutputPath)` such as `bin\Debug`. It appears the core MSBuild targets are leaving timestamps as-is in the `CopyFilesToOutputDirectory` and `_CopyFilesMarkedCopyLocal` targets. Assemblies from Xamarin.Forms NuGet are out of date here, but I don't think we should do anything, since the core MSBuild targets are doing this. For now, I removed the checks for `$(OutputPath)` in this test, and cleaned up the test a bit. Update `Files.ExtractAll()` so that the timestamps of extracted files are set to "now", in order to fix some failing unit tests on macOS.
@radekdoulik, my thoughts on the graph for startup time: #1150 is this commit. #1149 has a few commits, but most notably updates our emulator here from API 21 to API 28. This commit also has a slower startup, we seem to have a lot of variance now... I'm wondering if there is a slower code path in Xamarin.Forms for newer API levels? I've been testing 15.9 today, and seems like XamlC is still working fine? I'm not sure what could have any impact to startup time here.... |
Yes, might be related to the emulator updates. I also plan to fix the Bcl test measurements to see if it might be fluctuating as well? (it is longer running test exercising more of our code) Which might give us better indication whether it is emulator related or Xamarin.Forms related. |
Context: dotnet#2088 Context: dotnet#2129 Context: dotnet#2199 In 4deec52, I fixed the #deletebinobj problem we discovered. However... I introduced a regression to incremental builds, I noticed that the `_CompileJava` target is now running on a second build with no changes. Third build? oddly it gets skipped... It seems to be due to our use of flag files: 1. `_UpdateAndroidResgen` updates `R.cs.flag` and uses the file as an output 2. `_GenerateJavaDesignerForComponent` now uses `R.cs.flag` as an input 3. `_GenerateJavaStubs` *also* updates the timestamp on `R.cs.flag`. This was added in 970da9e, as a workaround for our two instances of `ConvertResourcesCases`. 4. `_GenerateJavaDesignerForComponent` will now run again on the next build. Since 1886e6f eliminated the second call to `ConvertResourcesCases`, we don't need to update `R.cs.flag` in no. 3 any longer. Removing the call to `<Touch />` `R.cs.flag` in `_GenerateJavaStubs` fixed the issue, and I added some assertions in relevant tests to check that the `_CompileJava` and `_GenerateJavaDesignerForComponent` targets aren't running on incremental builds.
Context: #2088 Context: #2129 Context: #2199 In 4deec52, I fixed the #deletebinobj problem we discovered. However... I introduced a regression to incremental builds, I noticed that the `_CompileJava` target is now running on a second build with no changes. Third build? oddly it gets skipped... It seems to be due to our use of flag files: 1. `_UpdateAndroidResgen` updates `R.cs.flag` and uses the file as an output 2. `_GenerateJavaDesignerForComponent` now uses `R.cs.flag` as an input 3. `_GenerateJavaStubs` *also* updates the timestamp on `R.cs.flag`. This was added in 970da9e, as a workaround for our two instances of `ConvertResourcesCases`. 4. `_GenerateJavaDesignerForComponent` will now run again on the next build. Since 1886e6f eliminated the second call to `ConvertResourcesCases`, we don't need to update `R.cs.flag` in no. 3 any longer. Removing the call to `<Touch />` `R.cs.flag` in `_GenerateJavaStubs` fixed the issue, and I added some assertions in relevant tests to check that the `_CompileJava` and `_GenerateJavaDesignerForComponent` targets aren't running on incremental builds.
[Xamarin.Android.Build.Tasks] fix incremental builds for Xamarin.Forms projects
@StephaneDelcroix noticed a build performance problem in Xamarin.Forms
projects:
_UpdateAndroidResgen
and_GenerateJavaStubs
run again!
The cause appeared to be this:
I modified the
CheckTimestamps
test to replicate this issue:Tabbar.axml
from the Xamarin.Forms templateThis surfaced several problems with timestamps and Xamarin.Android
building incrementally...
Problem 1
In the
_GenerateJavaStubs
target, theConvertResourcesCases
MSBuild task is not supplied a
AndroidConversionFlagFile
. It usesthis file to compare timestamps and decide if XML files need fixed up
or not.
Supplying
$(_AndroidResgenFlagFile)
seemed to be the way to go here,since
_UpdateAndroidResgen
runs right before this task. It alsosolves the original problem from the
Tabbar.axml
file.I also realized we should be using a "stamp" file for the
_GenerateJavaStubs
MSBuild target, in general.This gives us several benefits:
ConvertResourcesCases
has a properAndroidConversionFlagFile
itcan use for checking timestamps
Outputs
of the target are drastically simplified, this shouldhelp peformance in MSBuild evaluating if the target should run
So I added a new file in
$(IntermediateOutputPath)
,obj\Debug\_javastubs.stamp
we use to determine if_GenerateJavaStubs
should run.I also tried to do things "the right way", such as:
FileWrites
item groupClean
After these changes, two instances of
ConvertResourcesCases
werestill fighting each other. They would always run and update the
timestamps on one file: causing the other one to run.
ConvertResourcesCases
runs twice:_UpdateAndroidResgen
target_GenerateJavaStubs
targetBut these are both working with the same files, so in the second case,
we need to make sure
_GenerateJavaStubs
updates the timestamp on thefirst flag file so the target doesn't run every time.
Problem 2
Timestamps of Xamarin.Forms assemblies in
obj\Debug\linksrc
were outof date!
I updated the
_CopyIntermediateAssemblies
target to update thetimestamps of all files within this directory. It did not appear to be
doing it correctly, and was only running
<Touch />
on a subset ofassemblies.
Problem 3
After fixing No. 2, the next problem I noticed were
*.pdb
and*.mdb
files inobj\Debug\linksrc
were out of date!The problem here was a call to
<Touch />
:The
_DebugFilesCopiedToLinkerSrc
did not appear to contain anyitems!
I updated the
_CopyPdbFiles
and_CopyMdbFiles
targets to use thecorrect item groups.
Problem 4
Lastly, after fixing the previous three issues, the timestamps of
output assemblies in
obj\Release\android\assets
were not correct forRelease
builds.I had made a change for this in the past, which was making
Debug
builds work correctly:
#2028
At the time I wrote #2028, since the test wasn't using Xamarin.Forms,
I did not see the problem ocurring in
Release
mode. It does occurif you are using NuGet packages.
I replicated what I changed in the
_LinkAssembliesNoShrink
target,and also renamed the item groups to be unique:
_LinkAssembliesNoShrinkFiles
_LinkAssembliesShrinkFiles
Other changes
The
CheckTimestamps
test also was verifying timestamps in$(OutputPath)
such asbin\Debug
.It appears the core MSBuild targets are leaving timestamps as-is in
the
CopyFilesToOutputDirectory
and_CopyFilesMarkedCopyLocal
targets. Assemblies from Xamarin.Forms NuGet are out of date here, but
I don't think we should do anything, since the core MSBuild targets
are doing this.
For now, I removed the checks for
$(OutputPath)
in this test, andcleaned up the test a bit.