-
Notifications
You must be signed in to change notification settings - Fork 533
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] Install should skip Build when inside the IDE #2626
[Xamarin.Android.Build.Tasks] Install should skip Build when inside the IDE #2626
Conversation
I like this idea. We need to ping @sgmunn to see what the impact on VSForMac would be. Oh and see if we can fix the 248 failed tests :P 2 second saving is awesome! |
Not sure I completely understand this. but what I think I got is that because the IDE generally builds the project before install, then invokes the install target that we can skip build during install. we are assuming therefore that inside of VS that the build has been done when we invoke the install target. I think that makes sense, one side-effect benefit is that the IDE option to execute again without building will, actually, not build again. |
@sgmunn your reading of this PR is correct. Do you think any work would be needed on the Mac side of things to allow this to work? or will it just work as is now? |
I don't think there would be any work, I am pretty sure that it would work out of the box. |
04bae28
to
0f7d405
Compare
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BuildHelper.cs
Show resolved
Hide resolved
…he IDE Context: https://github.com/xamarin/xamarin-android/wiki/IDE-Performance-Results Context: https://github.com/jonathanpeppers/HelloWorld When doing some performance measurements *inside* of Visual Studio, I noticed we seem to have significant overhead in a build with no changes. In a "Hello World" Xamarin.Forms app: Preparation Time: 00:03.9 Launch Time: 00:02.5 Where `Preparation Time` is everything MSBuild, and `Launch Time` is the time it takes to start the Android application and attach the debugger. `Preparation Time` is effectively: msbuild Foo.Android.csproj /p:BuildingInsideVisualStudio=True /t:Build msbuild Foo.Android.csproj /p:BuildingInsideVisualStudio=True /t:Install One concern here is that `Install` depends on `SignAndroidPackage`, which depends on `Build`. We are doing a lot of MSBuild work here twice, since MSBuild needs to run through certain targets twice and decide they can be skipped. This work is "not free" and mostly involved MSBuild evaluating properties and time stamps on files. What I found we could do here is skip `Build` on the `Install` target when `$(BuildingInsideVisualStudio)` is `True`. Due to the dependency chain, this also affects `SignAndroidPackage`. The minimal list of targets for `SignAndroidPackage` that still work: - `_CreatePropertiesCache` - `ResolveReferences` - `_CopyPackage` - `_Sign` Initial results from the IDE show: Preparation Time: 00:02.06s This is a ~2 second saving on the inner dev loop! ~~ MSBuild Tests ~~ Since our MSBuild tests set `$(BuildingInsideVisualStudio)`, a lot of our tests might break. We might have to add an additional call to `Build` in each failing test. The way I worked around this was to make the `CreateApkBuilder` method run `Build,SignAndroidPackage` by default. Originally there were 248 test failures. I also did some other cleanup: - Added a `ProjectBuilder.RunTarget` method, so tests can more easily run custom targets and not modify the `Target` property: `builder.RunTarget ("Foo")`. - Added `ProjectBuilder.DesignTimeBuild`. - `Builder` now has a `BuildingInsideVisualStudio` property to toggle the value as a command-line global property. We were putting this in the `csproj`, which seems a bit different than what IDEs are actually doing... Also added a `BuildOutsideVisualStudio` test to verify the `SignAndroidPackage` target works by itself *outside* of IDEs.
0f7d405
to
6f3a6ee
Compare
I manually tested the I felt the devloop was snappier, but I wasn't sure if there is a way to time the difference inside VS for Mac? |
VSMac has telemetry for the Build and Clean MSBuild targets - not sure if that is enough here since other targets are not measured. |
@sgmunn @mrward VS Windows has a telemetry event that measures the time it takes from the user hitting play (F5), to the debugger attached. Keeps up with time spent in MSBuild vs app startup and debugger. Maybe adding an overall telemetry event to measure the "devloop" like this would be good in VS for Mac? Probably helpful for iOS, too. |
There's a build and deploy telemetry event which @iainx added but I do not know much about it. There is also some debugger telemetry events but they do not include the build time separately. |
Tests using their own `LocalBuilder` should default `BuildingInsideVisualStudio` to `false`.
Error is:
Same happened on this PR: #2634 |
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.
Looks ok.
The analysis is incorrect; the This appears to be an actual error:
This is a reoccurring issue within There's also this, which is a bit more concerning:
Looks like
Unfortunately, I don't see an error message from (Now that I think of it, maybe I don't see why this PR would break |
Context: https://github.com/xamarin/xamarin-android/wiki/IDE-Performance-Results
Context: https://github.com/jonathanpeppers/HelloWorld
When doing some performance measurements inside of Visual Studio, I
noticed we seem to have significant overhead in a build with no
changes.
In a "Hello World" Xamarin.Forms app:
Where
Preparation Time
is everything MSBuild, andLaunch Time
isthe time it takes to start the Android application and attach the
debugger.
Preparation Time
is effectively:One concern here is that
Install
depends onSignAndroidPackage
,which depends on
Build
. We are doing a lot of MSBuild work heretwice, since MSBuild needs to run through certain targets twice and
decide they can be skipped. This work is "not free" and mostly
involved MSBuild evaluating properties and time stamps on files.
What I found we could do here is skip
Build
on theInstall
targetwhen
$(BuildingInsideVisualStudio)
isTrue
. Due to the dependencychain, this also affects
SignAndroidPackage
.The minimal list of targets for
SignAndroidPackage
that still work:_CreatePropertiesCache
ResolveReferences
_CopyPackage
_Sign
Initial results from the IDE show:
This is a ~2 second saving on the inner dev loop!
MSBuild Tests
Since our MSBuild tests set
$(BuildingInsideVisualStudio)
, a lot ofour tests might break. We might have to add an additional call to
Build
in each failing test.The way I worked around this was to make the
CreateApkBuilder
methodrun
Build,SignAndroidPackage
by default. Originally there were 248test failures.
I also did some other cleanup:
ProjectBuilder.RunTarget
method, so tests can more easilyrun custom targets and not modify the
Target
property:builder.RunTarget ("Foo")
.ProjectBuilder.DesignTimeBuild
.Builder
now has aBuildingInsideVisualStudio
property to togglethe value as a command-line global property. We were putting this in
the
csproj
, which seems a bit different than what IDEs areactually doing...
Also added a
BuildOutsideVisualStudio
test to verify theSignAndroidPackage
target works by itself outside of IDEs.