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] prevent _BuildLibraryProjectImportsCache always running #2132

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Aug 31, 2018

When testing a build with no changes, I started noticing:

Building target "_BuildLibraryImportsCache" completely.
Input file "App7\bin\Debug\netstandard2.0\App7.dll" is newer than output file "obj\Debug\90\libraryimports.cache".

What was weird, is that earlier in the build log:

Did not copy from file "bin\Debug\netstandard2.0\App7.dll" to file "bin\Debug\App7.dll" because the "SkipUnchangedFiles" parameter was set to "true" in the project and the files' sizes and timestamps match.

So the NetStandard assembly didn't change.

When I looked at these files:

8/31/2018  11:02 AM App7.dll
8/31/2018  10:09 AM resourcepaths.cache

Weird? It looks like resourcepaths.cache's timestamp needs to be
updated?

Then I looked and noticed that the timestamp of resourcepaths.cache
would only be updated if the XML changed.

My first attempt to fix this:

  • I made XDocumentExtensions.SaveIfChanged return a bool
    indicating if the file changed.
  • I changed the GetImportedLibraries task so that it would update
    the timestamp in cases where the file didn't change.

Unfortunately, that made _UpdateAndroidResgen run all the time,
which would be even worse!

So my second attempt was much better:

  • Add a $(_AndroidLibraryImportsCache).stamp file, to use as the
    Outputs of _BuildLibraryImportsCache.
  • This file's timestamp can operate independently of
    $(_AndroidLibraryImportsCache)

_BuildLibraryImportsCache now builds incrementally and gets skipped
properly:

Skipping target "_BuildLibraryImportsCache" because all output files are up-to-date with respect to the input files.

It seems to me this problem was always occurring on builds with no
changes. So this will likely help any incremental build.

Before this change:

146 ms  _BuildLibraryImportsCache                  1 calls
Total time: 2.824s

After this change:

  4 ms  _BuildLibraryImportsCache                  1 calls
Total time: 2.698s

I also updated an existing test to make sure the
_BuildLibraryImportsCache target isn't running all the time.

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

Need to think about this one more next week:

 Building target "_UpdateAndroidResgen" completely.
 Input file "obj\Debug\90\libraryimports.cache" is newer than output file "obj\Debug\90\R.cs.flag".

This is happening and shouldn't ^^

So it's as if the timestamp needs to be set to the date of the oldest input to _BuildLibraryImportsCache. I'm not sure if we currently have anything that does this right now.

…e always running

When testing a build with no changes, I started noticing:

    Building target "_BuildLibraryImportsCache" completely.
    Input file "App7\bin\Debug\netstandard2.0\App7.dll" is newer than output file "obj\Debug\90\libraryimports.cache".

What was weird, is that earlier in the build log:

    Did not copy from file "bin\Debug\netstandard2.0\App7.dll" to file "bin\Debug\App7.dll" because the "SkipUnchangedFiles" parameter was set to "true" in the project and the files' sizes and timestamps match.

So the NetStandard assembly didn't change.

When I looked at these files:

    8/31/2018  11:02 AM App7.dll
    8/31/2018  10:09 AM resourcepaths.cache

Weird? It looks like `resourcepaths.cache`'s timestamp needs to be
updated?

Then I looked and noticed that the timestamp of `resourcepaths.cache`
would only be updated if the XML changed.

My *first* attempt to fix this:
- I made `XDocumentExtensions.SaveIfChanged` return a `bool`
  indicating if the file changed.
- I changed the `GetImportedLibraries` task so that it would update
  the timestamp in cases where the file didn't change.

Unfortunately, that made `_UpdateAndroidResgen` run all the time,
which would be even worse!

So my *second* attempt was much better:
- Add a `$(_AndroidLibraryImportsCache).stamp` file, to use as the
  `Outputs` of `_BuildLibraryImportsCache`.
- This file's timestamp can operate independently of
  `$(_AndroidLibraryImportsCache)`

`_BuildLibraryImportsCache` now builds incrementally and gets skipped
properly:

    Skipping target "_BuildLibraryImportsCache" because all output files are up-to-date with respect to the input files.

It seems to me this problem was always occurring on builds with no
changes. So this will likely help any incremental build.

Before this change:

    146 ms  _BuildLibraryImportsCache                  1 calls
    Total time: 2.824s

After this change:

      4 ms  _BuildLibraryImportsCache                  1 calls
    Total time: 2.698s

I also updated an existing test to make sure the
`_BuildLibraryImportsCache` target isn't running all the time.
@jonathanpeppers jonathanpeppers force-pushed the buildlibraryimportscache branch from edf7edc to 9a41697 Compare September 4, 2018 18:40
@jonathanpeppers
Copy link
Member Author

I have another idea that seems like it will work, hoping it's green this time.

Build logs: BuildLibraryProjectImports.zip

@jonathanpeppers jonathanpeppers removed the do-not-merge PR should not be merged. label Sep 5, 2018
@dellis1972
Copy link
Contributor

@jonathanpeppers this good to go?

@jonathanpeppers
Copy link
Member Author

@dellis1972 yes, the second approach doesn’t seem break anything. Should be good to go.

@dellis1972 dellis1972 merged commit 87bb898 into dotnet:master Sep 5, 2018
jonpryor pushed a commit that referenced this pull request Sep 5, 2018
…e always running (#2132)

When testing a build with no changes, I started noticing:

    Building target "_BuildLibraryImportsCache" completely.
    Input file "App7\bin\Debug\netstandard2.0\App7.dll" is newer than output file "obj\Debug\90\libraryimports.cache".

What was weird, is that earlier in the build log:

    Did not copy from file "bin\Debug\netstandard2.0\App7.dll" to file "bin\Debug\App7.dll" because the "SkipUnchangedFiles" parameter was set to "true" in the project and the files' sizes and timestamps match.

So the NetStandard assembly didn't change.

When I looked at these files:

    8/31/2018  11:02 AM App7.dll
    8/31/2018  10:09 AM resourcepaths.cache

Weird? It looks like `resourcepaths.cache`'s timestamp needs to be
updated?

Then I looked and noticed that the timestamp of `resourcepaths.cache`
would only be updated if the XML changed.

My *first* attempt to fix this:
- I made `XDocumentExtensions.SaveIfChanged` return a `bool`
  indicating if the file changed.
- I changed the `GetImportedLibraries` task so that it would update
  the timestamp in cases where the file didn't change.

Unfortunately, that made `_UpdateAndroidResgen` run all the time,
which would be even worse!

So my *second* attempt was much better:
- Add a `$(_AndroidLibraryImportsCache).stamp` file, to use as the
  `Outputs` of `_BuildLibraryImportsCache`.
- This file's timestamp can operate independently of
  `$(_AndroidLibraryImportsCache)`

`_BuildLibraryImportsCache` now builds incrementally and gets skipped
properly:

    Skipping target "_BuildLibraryImportsCache" because all output files are up-to-date with respect to the input files.

It seems to me this problem was always occurring on builds with no
changes. So this will likely help any incremental build.

Before this change:

    146 ms  _BuildLibraryImportsCache                  1 calls
    Total time: 2.824s

After this change:

      4 ms  _BuildLibraryImportsCache                  1 calls
    Total time: 2.698s

I also updated an existing test to make sure the
`_BuildLibraryImportsCache` target isn't running all the time.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 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.

2 participants