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.

Sign up for GitHub

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

Jump to bottom

Fix an issue with incremental builds. #9183

Merged
merged 8 commits into from
Aug 20, 2024
Merged

Fix an issue with incremental builds. #9183

merged 8 commits into from
Aug 20, 2024

Conversation

dellis1972
Copy link
Contributor

@dellis1972 dellis1972 commented Aug 8, 2024 •
edited
Loading

We have been getting on and off reports of the following errors in Android for quite a while now.

Resources/values/styles.xml(2): error APT2260: resource attr/colorPrimary (aka Microsoft.Maui:attr/colorPrimary) not found.

It was always very confusing as to why this was happening. No one ever seemed to be able to figure out why these files were deleted.

Well it turns out they were not deleted, just not included. While building maui I came across this issue and managed to capture a .binlog.

It turns out that the _CleanMonoAndroidIntermediateDir target was running. And it was deleting certain files. This target was running because _CleanIntermediateIfNeeded was running. That was running because the build.props file had changed... That was because the NuGet project.assets.json file had a new timestamp! On an incremental build....

So it looks like NuGet sometimes touches the project.assets.json file even if it does not change. We can't blame them for that as we do similar things.

The next confusing thing and the principal cause is that the libraryprojectimports.cache was not present. However the _ResolveLibraryProjectImports.stamp file was. This means the _ResolveLibraryProjectImports does NOT run. So we end up NOT including ANY of the resource res.zip files when calling aapt2. The _ResolveLibraryProjectImports target did not include libraryprojectimports.cache in its Outputs. So if the file did not exist, but the stamp file did the target would not run. It is confusing because in the _CleanMonoAndroidIntermediateDir we delete the libraryprojectimports.cache and the entire $(IntermediateOutputPath)stamp directory..

After much seaerching I did something while maui was opened in VSCode. I deleted its artifacts directory to clean up and start a new build and ..... it magically re-appeared!!! Then I thought... Design Time Builds!!!!

So the problem we have is this. Our current system the Design Time Builds and the main builds all use the same stamp files!!! So what was happening is a design time build was running before the main build, and it was creating a design time cache file... but a stamp IN THE SAME LOCATION as the main build would. As a result the main build would skip the _ResolveLibraryProjectImports target COMPLETELY even if the output cache file was not present. Once that happens it will NEVER recover until you delete the bin/obj directories. This only happened occasionally because it would depend on if you have the project open in a IDE and when the IDE decided to run a Design Time Build.

So lets give the design time build its own stamp directory. It might cause some targets to run during a design time build but I think that is prefered to a failing main build. To work around the NuGet causing a build when it touches a the project.assets.json file we now use a Hash. This means we only refresh the library projects if the contents of the project.assets.json file change, rather than the timestamp. Also there was a bug where we would automatically fallback to the NuGet lock file even if we found the project.assets.json file.

@dellis1972 dellis1972 force-pushed the dev/dellis1972/aapt2incrementalerror branch from f68cfc4 to 5df9257 Compare August 8, 2024 15:46
@dellis1972 dellis1972 requested a review from jonpryor August 8, 2024 15:47
@dellis1972 dellis1972 force-pushed the dev/dellis1972/aapt2incrementalerror branch from 5df9257 to 548e76f Compare August 8, 2024 15:51
@dellis1972
Copy link
Contributor Author

Need to switch out the ComputeHash for https://learn.microsoft.com/en-us/visualstudio/msbuild/getfilehash-task?view=vs-2022 as ComputeHash just hashes the filename not the contents

@dellis1972
Copy link
Contributor Author

I'm gonna back out the RemoveDir changes and do that in another PR.

@dellis1972 dellis1972 force-pushed the dev/dellis1972/aapt2incrementalerror branch from b44e5ea to a7b67c8 Compare August 9, 2024 07:38
@dellis1972
Copy link
Contributor Author

@jonathanpeppers I'm attaching the log output. See if you can see something I've missed.

builderror.log.zip

@jonathanpeppers
Copy link
Member

@dellis1972 is there a .binlog? If it's MAUI they might save the log for you in artifacts/logs.

@dellis1972
Copy link
Contributor Author

@dellis1972 is there a .binlog? If it's MAUI they might save the log for you in artifacts/logs.

Long gone I'm afraid, I've nuke'd the checkout a few times since then. Now I can't repo.

@dellis1972 dellis1972 marked this pull request as draft August 13, 2024 13:05
jonathanpeppers
jonathanpeppers reviewed Aug 13, 2024
View reviewed changes
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offhand, I can't think of anything that would go wrong if we had a separate set of stamp files for DTBs. Do they get deleted on a Clean?

The CustomDesignerTargetSetupDependenciesForDesigner test failed, but it's mixing DTBs and regular builds. You might just update that test for now.

@dellis1972
Copy link
Contributor Author

Offhand, I can't think of anything that would go wrong if we had a separate set of stamp files for DTBs. Do they get deleted on a Clean?

We delete the entire designtime directory, so all the stamps will be removed then.

The CustomDesignerTargetSetupDependenciesForDesigner test failed, but it's mixing DTBs and regular builds. You might just update that test for now.

Yup there are a few tests I need to update.

@dellis1972 dellis1972 force-pushed the dev/dellis1972/aapt2incrementalerror branch from ae0005e to 39a1678 Compare August 16, 2024 09:01
@dellis1972
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dellis1972
Fix an issue with incremental builds.
d954588 
We have been getting on and off reports of the following
errors in Android for quite a while now.

```
Resources/values/styles.xml(2): error APT2260: resource attr/colorPrimary (aka Microsoft.Maui:attr/colorPrimary) not found.
```

It was always very confusing as to why this was happening.
No one ever seemed to be able to figure out why these files
were deleted.

Well it turns out they were not deleted, just not included.
While building maui I came across this issue and managed to
capture a `.binlog`.

It turns out that the `_CleanMonoAndroidIntermediateDir` target
was running. And it was deleting certain files. This target was
running because `_CleanIntermediateIfNeeded` was running.
That was running because the `build.props` file had changed...
That was because the NuGet `project.assets.json` file had a new
timestamp! On an incremental build....

So it looks like NuGet sometimes touches the `project.assets.json`
file even if it does not change. We can't blame them for that as
we do similar things.

The next confusing thing and the principal cause is that the
`libraryprojectimports.cache` was being deleted, probably buy the
call to `_CleanMonoAndroidIntermediateDir`. However the
`_ResolveLibraryProjectImports.stamp` file was left in place.
This means the `_ResolveLibraryProjectImports` does NOT run.
So we end up NOT including ANY of the resource `res.zip` files when
calling `aapt2`. The `_ResolveLibraryProjectImports` target did not
include `libraryprojectimports.cache` in its `Outputs`. So if the file
did not exist, but the stamp file did the target would not run.
It is confusing because in the `_CleanMonoAndroidIntermediateDir` we delete
the `libraryprojectimports.cache` and the entire `$(IntermediateOutputPath)stamp`
directory..

So lets include `libraryprojectimports.cache` in the outputs for starters.
Then update the `_CreatePropertiesCache` target to use a hash of the
`project.assets.json` file rather than a timestamp. This way we really
only run the build if something actually changes.
@dellis1972
try this
b138f5f
@dellis1972
try to give dtb their own stamps directory
dfe39a3
@dellis1972
Add clean up for design time builds
47e7625
@dellis1972
Try to fix the tests
bd521d1
@dellis1972
Fix CustomDesignerTargetSetupDependenciesForDesigner Test
709246f
@dellis1972
Fix another test
da860a9
@dellis1972
rework the change
596916c
@dellis1972 dellis1972 force-pushed the dev/dellis1972/aapt2incrementalerror branch from 39a1678 to 596916c Compare August 19, 2024 09:37
@dellis1972 dellis1972 marked this pull request as ready for review August 19, 2024 09:41
jonathanpeppers
jonathanpeppers approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK, the case for using the same stamp files were something like:

  • I open an .xml file in the Android designer
  • It runs a bunch of targets to support the designer
  • Next incremental build is able to skip a few steps

Since, we are less concerned about the designer, this is probably fine now.

@jonpryor jonpryor merged commit 3669ea5 into main Aug 20, 2024
58 checks passed
@jonpryor jonpryor deleted the dev/dellis1972/aapt2incrementalerror branch August 20, 2024 18:10
jonpryor pushed a commit that referenced this pull request Aug 21, 2024
@dellis1972 @jonpryor
[Xamarin.Android.Build.Tasks] Fix an issue with incremental builds (#…
ebd5b18 
…9183)

We have been getting on and off reports of the following build errors
in .NET for Android for quite a while now:

	Resources/values/styles.xml(2): error APT2260: resource attr/colorPrimary (aka Microsoft.Maui:attr/colorPrimary) not found.

It was always very confusing as to why this was happening.  No one
ever seemed to be able to figure out why these files were deleted.

Well it turns out they were not deleted, just not included. 
While building dotnet/maui I came across this issue and managed to
capture a `.binlog`.

It turns out that the `_CleanMonoAndroidIntermediateDir` target was
running, and it was deleting certain files.  This target was running
because `_CleanIntermediateIfNeeded` was running.  That was running
because the `build.props` file had changed, which was because the
NuGet `project.assets.json` file had a new timestamp!

On an incremental build!

So it looks like NuGet sometimes touches the `project.assets.json`
file even if it does not change.  We can't blame them for that as we
do similar things.

The next confusing thing and the principal cause is that the
`libraryprojectimports.cache` was not present.  However, the
`_ResolveLibraryProjectImports.stamp` file *was* present.  This means
that the `_ResolveLibraryProjectImports` target does NOT run, so we
end up NOT including ANY of the resource `res.zip` files when calling
`aapt2`.  The `_ResolveLibraryProjectImports` target did not include
`libraryprojectimports.cache` in its `Outputs`, so if the file did
not exist, but the stamp file *did*, then the target would not run.

It is confusing because in the `_CleanMonoAndroidIntermediateDir` we
delete the `libraryprojectimports.cache` and the entire
`$(IntermediateOutputPath)stamp` directory.

After much searching I did something while maui was opened in VSCode:
I deleted its `artifacts` directory to clean up and start a new build
and ..... it magically re-appeared!!!

Then I thought... Design Time Builds!!!!

So the problem we have is this: in our current system the
Design Time Builds and the main builds all use the same stamp files!!!
What was happening is a design time build was running before the main
build, and it was creating a design time cache file into
`$(_AndroidIntermediateDesignTimeBuildDirectory)`
(`$(IntermediateOutputPath)designtime`), but place the stamp files
INTO THE SAME LOCATION as the main build would.  As a result the main
build would skip the `_ResolveLibraryProjectImports` target
COMPLETELY even if the output cache file was not present.  Once that
happens it will NEVER recover until you delete the bin/obj directories.

This only happened occasionally because it would depend on if you
have the project open in a IDE and when the IDE decided to run a
Design Time Build.

Fix this by giving design-time builds their own stamp directory.
It might cause some targets to run during a design time build but I
think that is preferred to a failing main build.

To work around the NuGet causing a build when it touches a the
`project.assets.json` file we now use `<GetFileHash/>`.  This means
we only refresh the library projects if the contents of the
`project.assets.json` file change, rather than the timestamp.

Finally, there was a bug where we would automatically fallback to the
NuGet lock file even if we found the `project.assets.json` file.
jonathanpeppers added a commit that referenced this pull request Aug 22, 2024
@jonathanpeppers
Bump to 34.0.137 of the .NET 8 Android workload
ec9d7ef 
Changes: 34.0.113...b0fd011

.NET 8 changelog:

* [Mono.Android] AndroidMessageHandler should follow HTTP-308 redirects (#8951)
* [ci] Fix android source path for MAUI test job (#9030)
* [ci] Update checkout path for nightly build (#9028)
* [Mono.Android-Tests] Fix repo URL in redirect tests (#9035)
* [Xamarin.Android.Build.Tasks] Support VS "Build Acceleration" (#9042)
* [xaprepare] Always use release mono bundle (#9106)
* [ci] Update sdk-insertions trigger to manual only (#9029)
* Bump to dotnet/runtime@4a37e7305c 8.0.8 (#9033)
* Bump to dotnet/installer@b638a84fba 8.0.205-servicing.24212.27 (#9032)
* [ci] Disable CodeQL on macOS, Linux, non-main jobs (#9111)
* [ci] Use DotNetCoreCLI to sign macOS files (#9102)
* [tests] fix `InvalidTargetPlatformVersion` for `net8.0` (#9110)
* [ci] Run "Push Internal" job on AzurePipelines-EO pool (#8991)
* Bump to dotnet/runtime@2d5c0b720c 8.0.8 (#9123)
* [Mono.Android] Data sharing and Close() overrides (#9103)
* [Xamarin.Android.Build.Tasks] fix `Inputs` for `_Generate*Java*` targets (#9174)
* Bump to xamarin/monodroid@e6a7cf474a (#9193)
* [release/8.0.4xx] Backport maestro and artifact drop infra improvements (#9195)
* Bump to dotnet/runtime@62f69f1e86 8.0.9 (#9191)
* Bump to dotnet/runtime@ed13b35174 8.0.9 (#9221)
* [build] Update package metadata (#9230)
* [Xamarin.Android.Build.Tasks] Rethink default property values (#9155) (#9224)
* [Xamarin.Android.Build.Tasks] Fix an issue with incremental builds (#9183)
* [ci] Improve push_signed_nugets job condition (#9240)
jonathanpeppers added a commit that referenced this pull request Aug 23, 2024
@jonathanpeppers
Bump to 34.0.137 of the .NET 8 Android workload (#9243)
119d46c 
Changes: 34.0.113...b0fd011

.NET 8 changelog:

* [Mono.Android] AndroidMessageHandler should follow HTTP-308 redirects (#8951)
* [ci] Fix android source path for MAUI test job (#9030)
* [ci] Update checkout path for nightly build (#9028)
* [Mono.Android-Tests] Fix repo URL in redirect tests (#9035)
* [Xamarin.Android.Build.Tasks] Support VS "Build Acceleration" (#9042)
* [xaprepare] Always use release mono bundle (#9106)
* [ci] Update sdk-insertions trigger to manual only (#9029)
* Bump to dotnet/runtime@4a37e7305c 8.0.8 (#9033)
* Bump to dotnet/installer@b638a84fba 8.0.205-servicing.24212.27 (#9032)
* [ci] Disable CodeQL on macOS, Linux, non-main jobs (#9111)
* [ci] Use DotNetCoreCLI to sign macOS files (#9102)
* [tests] fix `InvalidTargetPlatformVersion` for `net8.0` (#9110)
* [ci] Run "Push Internal" job on AzurePipelines-EO pool (#8991)
* Bump to dotnet/runtime@2d5c0b720c 8.0.8 (#9123)
* [Mono.Android] Data sharing and Close() overrides (#9103)
* [Xamarin.Android.Build.Tasks] fix `Inputs` for `_Generate*Java*` targets (#9174)
* Bump to xamarin/monodroid@e6a7cf474a (#9193)
* [release/8.0.4xx] Backport maestro and artifact drop infra improvements (#9195)
* Bump to dotnet/runtime@62f69f1e86 8.0.9 (#9191)
* Bump to dotnet/runtime@ed13b35174 8.0.9 (#9221)
* [build] Update package metadata (#9230)
* [Xamarin.Android.Build.Tasks] Rethink default property values (#9155) (#9224)
* [Xamarin.Android.Build.Tasks] Fix an issue with incremental builds (#9183)
* [ci] Improve push_signed_nugets job condition (#9240)

~~ Other changes ~~

* Setup Maestro to track .NET8 builds

After setting up the appropriate changes in `Version.Details.xml`, I ran:

    > darc update-dependencies --id 235947
    Looking up build with BAR id 235947
    Updating 'Microsoft.Android.Sdk.Windows': '1' => '34.0.137' (from build '8.0.4xx-    b0fd011-1' of 'https://github.com/dotnet/android')
    Checking for coherency updates...
    Using 'Strict' coherency mode. If this fails, a second attempt utilizing 'Legacy' Coherency mode will be made.
    Local dependencies updated based on build with BAR id 235947 (8.0.4xx-b0fd0113c829edd9bd1bc7d742255a237ff19f69-1 from https://github.com/dotnet/android@release/8.0.4xx)
jonathanpeppers added a commit that referenced this pull request Aug 23, 2024
@jonathanpeppers
Bump to 34.0.137 of the .NET 8 Android workload (#9243)
bc56b3a 
Changes: 34.0.113...b0fd011

.NET 8 changelog:

* [Mono.Android] AndroidMessageHandler should follow HTTP-308 redirects (#8951)
* [ci] Fix android source path for MAUI test job (#9030)
* [ci] Update checkout path for nightly build (#9028)
* [Mono.Android-Tests] Fix repo URL in redirect tests (#9035)
* [Xamarin.Android.Build.Tasks] Support VS "Build Acceleration" (#9042)
* [xaprepare] Always use release mono bundle (#9106)
* [ci] Update sdk-insertions trigger to manual only (#9029)
* Bump to dotnet/runtime@4a37e7305c 8.0.8 (#9033)
* Bump to dotnet/installer@b638a84fba 8.0.205-servicing.24212.27 (#9032)
* [ci] Disable CodeQL on macOS, Linux, non-main jobs (#9111)
* [ci] Use DotNetCoreCLI to sign macOS files (#9102)
* [tests] fix `InvalidTargetPlatformVersion` for `net8.0` (#9110)
* [ci] Run "Push Internal" job on AzurePipelines-EO pool (#8991)
* Bump to dotnet/runtime@2d5c0b720c 8.0.8 (#9123)
* [Mono.Android] Data sharing and Close() overrides (#9103)
* [Xamarin.Android.Build.Tasks] fix `Inputs` for `_Generate*Java*` targets (#9174)
* Bump to xamarin/monodroid@e6a7cf474a (#9193)
* [release/8.0.4xx] Backport maestro and artifact drop infra improvements (#9195)
* Bump to dotnet/runtime@62f69f1e86 8.0.9 (#9191)
* Bump to dotnet/runtime@ed13b35174 8.0.9 (#9221)
* [build] Update package metadata (#9230)
* [Xamarin.Android.Build.Tasks] Rethink default property values (#9155) (#9224)
* [Xamarin.Android.Build.Tasks] Fix an issue with incremental builds (#9183)
* [ci] Improve push_signed_nugets job condition (#9240)

~~ Other changes ~~

* Setup Maestro to track .NET8 builds

After setting up the appropriate changes in `Version.Details.xml`, I ran:

    > darc update-dependencies --id 235947
    Looking up build with BAR id 235947
    Updating 'Microsoft.Android.Sdk.Windows': '1' => '34.0.137' (from build '8.0.4xx-    b0fd011-1' of 'https://github.com/dotnet/android')
    Checking for coherency updates...
    Using 'Strict' coherency mode. If this fails, a second attempt utilizing 'Legacy' Coherency mode will be made.
    Local dependencies updated based on build with BAR id 235947 (8.0.4xx-b0fd0113c829edd9bd1bc7d742255a237ff19f69-1 from https://github.com/dotnet/android@release/8.0.4xx)
grendello added a commit that referenced this pull request Aug 27, 2024
@grendello
Merge branch 'main' into dev/grendel/perfetto-integration
9d10266 
* main: (47 commits)
  Bump to dotnet/sdk@5642787dac 9.0.100-rc.2.24426.2 (#9247)
  LEGO: Merge pull request 9246
  Bump to 34.0.137 of the .NET 8 Android workload (#9243)
  Bump external/Java.Interop from `d30d554` to `51b784a` (#9241)
  Bump dotnet/android-tools@6575743 (#9235)
  Bump to mono/debugger-libs@d5664344 (#9238)
  [ci] Improve push_signed_nugets job condition (#9240)
  Bump to dotnet/android-tools@657574378a6 and xamarin/monodroid@8bd4bae7 (#9216)
  Bump to dotnet/java-interop@d30d554 (#9234)
  Localized file check-in by OneLocBuild Task (#9236)
  Bump to dotnet/sdk@e2b7b9d2b4 9.0.100-rc.2.24420.1 (#9228)
  $(AndroidPackVersionSuffix)=rc.2; net9 is 35.0.0-rc.2 (#9233)
  [Xamarin.Android.Build.Tasks] Scan for JCWs for each ABI in parallel (#9215)
  [Xamarin.Android.Build.Tasks] %(JavaArtifact) is a list (#9112)
  [native/monodroid] Fix error demangling satellite assembly names (#9166)
  [build] Update package metadata (#9230)
  [Xamarin.Android.Build.Tasks] Remove ILRepack (#9226)
  [Xamarin.Android.Build.Tasks] Fix an issue with incremental builds (#9183)
  [Xamarin.Android.Build.Tasks] remove `$XAMARIN_BUILD_ID` (#9223)
  [Mono.Android] Use .NET version of mdoc (#9225)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers approved these changes

@jonpryor jonpryor Awaiting requested review from jonpryor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dellis1972 @jonathanpeppers @jonpryor