-
Notifications
You must be signed in to change notification settings - Fork 531
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 w/ native assembler #3561
Merged
jonpryor
merged 1 commit into
dotnet:master
from
jonathanpeppers:incremental-nativeassembly
Aug 29, 2019
Merged
[Xamarin.Android.Build.Tasks] fix incremental builds w/ native assembler #3561
jonpryor
merged 1 commit into
dotnet:master
from
jonathanpeppers:incremental-nativeassembly
Aug 29, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When testing performance, I found an issue with `_GenerateJavaStubs`: 1. Change `MainActivity.cs` in an Android app project and hit F5. 2. The `Build` runs `_GenerateJavaStubs` 3. The `Install` runs `_GenerateJavaStubs` 4. Any subsequent build (even with no changes) will run `_GenerateJavaStubs`... The log reads: Building target "_GenerateJavaStubs" completely. Input file "obj\Debug\90\android\assets\App14.dll" is newer than output file "obj\Debug\90\android\typemap.jm.arm64-v8a.s". In 74cf0ec, we modified the `Outputs` to be: Outputs="$(_AndroidStampDirectory)_GenerateJavaStubs.stamp;@(_TypeMapAssemblySource)" This would be OK, except in some cases we don't update this file: MonoAndroidHelper.CopyIfStreamChanged (ms, asmFileName); If `_GenerateJavaStubs` runs, but the typemap doesn't change, we will be stuck in a spot where `_GenerateJavaStubs` will always run on incremental builds. `_GeneratePackageManagerJava` has the exact same problem as well. ~~ IncrementalClean ~~ The real issue 74cf0ec was trying to solve was that some of these `typemap.*.s` files were getting deleted during incremental builds. A PR build was hitting: _CompileNativeAssemblySources: Assembler messages: can't open typemap.mj.armeabi-v7a.s for reading: No such file or directory Due to: Skipping target "_GenerateJavaStubs" because all output files are up-to-date with respect to the input files. We suspected `IncrementalClean` was deleting these files, but we weren't able to find the right log file where that happened... Going through things again, I figured out what was going on: * `@(_EnvironmentAssemblySource)` and `@(_NativeAssemblyTarget)` were not added to `FileWrites` * `_CreateApplicationSharedLibraries` was not running during `Build` but `SignAndroidPackage`. This means it was running *after* `IncrementalClean`. Any files it produced that were in `FileWrites` are ignored... * We can run `_CreateApplicationSharedLibraries` right after `_CompileJava`. This has other benefits than correctness for `IncrementalClean`, such as when a build error occurs. The UX is a bit nicer in IDEs for errors from `Build` than `Install`. After these changes I can remove the extra `Outputs` from `_GenerateJavaStubs` and `_GeneratePackageManagerJava` and solely rely on the stamp files as an output. Moving `_CreateApplicationSharedLibraries` to `Build` should not impact incremental build performance, as it should be target that is skipped on almost all builds. I also added a new test for these scenarios, verifying targets skip and files are added to `FileWrites` appropriately. Other fixes: * `AndroidEnvironment` files were not an `Input` to `_GeneratePackageManagerJava`. So changing environment vars seems like it would always require a `Rebuild` to work...
grendello
approved these changes
Aug 29, 2019
jonpryor
pushed a commit
that referenced
this pull request
Sep 4, 2019
…ler (#3561) When testing performance, I found an issue with `_GenerateJavaStubs`: 1. Change `MainActivity.cs` in an Android app project and hit Run. 2. The `Build` runs `_GenerateJavaStubs` 3. The `Install` runs `_GenerateJavaStubs` 4. Any subsequent build (even with no changes) will run `_GenerateJavaStubs`... The log reads: Building target "_GenerateJavaStubs" completely. Input file "obj\Debug\90\android\assets\App14.dll" is newer than output file "obj\Debug\90\android\typemap.jm.arm64-v8a.s". In 74cf0ec, we modified the `Outputs` to be: Outputs="$(_AndroidStampDirectory)_GenerateJavaStubs.stamp;@(_TypeMapAssemblySource)" This would be OK, except in some cases we don't update this file: MonoAndroidHelper.CopyIfStreamChanged (ms, asmFileName); If `_GenerateJavaStubs` runs, but the typemap doesn't change, we will be stuck in a spot where `_GenerateJavaStubs` will always run on incremental builds. `_GeneratePackageManagerJava` has the exact same problem as well. ~~ IncrementalClean ~~ The real issue 74cf0ec was trying to solve was that some of these `typemap.*.s` files were getting deleted during incremental builds. A PR build was hitting: _CompileNativeAssemblySources: Assembler messages: can't open typemap.mj.armeabi-v7a.s for reading: No such file or directory Due to: Skipping target "_GenerateJavaStubs" because all output files are up-to-date with respect to the input files. We suspected `IncrementalClean` was deleting these files, but we weren't able to find the right log file where that happened... Going through things again, I figured out what was going on: * `@(_EnvironmentAssemblySource)` and `@(_NativeAssemblyTarget)` were not added to `@(FileWrites)` * `_CreateApplicationSharedLibraries` was not running during `Build` but rather in `SignAndroidPackage`. This means it was running *after* `IncrementalClean`. Any files it produced that were in `@(FileWrites)` are ignored... * We can run `_CreateApplicationSharedLibraries` right after `_CompileJava`. This has other benefits than correctness for `IncrementalClean`, such as when a build error occurs. The UX is a bit nicer in IDEs for errors from `Build` than `Install`. After these changes I can remove the extra `Outputs` from `_GenerateJavaStubs` and `_GeneratePackageManagerJava` and solely rely on the stamp files as an output. Moving `_CreateApplicationSharedLibraries` to `Build` should not impact incremental build performance, as it should be target that is skipped on almost all builds. I also added a new test for these scenarios, verifying targets skip and files are added to `FileWrites` appropriately. Other fixes: * `@(AndroidEnvironment)` files were not an `Input` to the `_GeneratePackageManagerJava` target. So changing environment vars seemslike it would always require a `Rebuild` to work...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When testing performance, I found an issue with
_GenerateJavaStubs
:MainActivity.cs
in an Android app project and hit F5.Build
runs_GenerateJavaStubs
Install
runs_GenerateJavaStubs
_GenerateJavaStubs
...The log reads:
In 74cf0ec, we modified the
Outputs
to be:This would be OK, except in some cases we don't update this file:
If
_GenerateJavaStubs
runs, but the typemap doesn't change, we willbe stuck in a spot where
_GenerateJavaStubs
will always run onincremental builds.
_GeneratePackageManagerJava
has the exact same problem as well.IncrementalClean
The real issue 74cf0ec was trying to solve was that some of these
typemap.*.s
files were getting deleted during incremental builds.A PR build was hitting:
Due to:
We suspected
IncrementalClean
was deleting these files, but weweren't able to find the right log file where that happened...
Going through things again, I figured out what was going on:
@(_EnvironmentAssemblySource)
and@(_NativeAssemblyTarget)
werenot added to
FileWrites
_CreateApplicationSharedLibraries
was not running duringBuild
but
SignAndroidPackage
. This means it was running afterIncrementalClean
. Any files it produced that were inFileWrites
are ignored...
_CreateApplicationSharedLibraries
right after_CompileJava
. This has other benefits than correctness forIncrementalClean
, such as when a build error occurs. The UX is abit nicer in IDEs for errors from
Build
thanInstall
.After these changes I can remove the extra
Outputs
from_GenerateJavaStubs
and_GeneratePackageManagerJava
and solely relyon the stamp files as an output.
Moving
_CreateApplicationSharedLibraries
toBuild
should notimpact incremental build performance, as it should be target that is
skipped on almost all builds.
I also added a new test for these scenarios, verifying targets skip
and files are added to
FileWrites
appropriately.Other fixes:
AndroidEnvironment
files were not anInput
to_GeneratePackageManagerJava
. So changing environment vars seemslike it would always require a
Rebuild
to work...