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] <CopyIfChanged/> should check size #3405

Merged
merged 2 commits into from Aug 1, 2019

Conversation

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Jul 29, 2019

[Xamarin.Android.Build.Tasks] should check size

Fixes: #2584

Building a project with two different $(TargetFrameworkVersion):

msbuild /p:Configuration=Release /p:TargetFrameworkVersion=v4.4
msbuild /t:Configuration=Release /p:TargetFrameworkVersion=v9.0

Results in a build error:

error MSB4018: The "LinkAssemblies" task failed unexpectedly.
Mono.Linker.MarkException: Error processing method: 'System.Void BatchStepSensor.BatchStepSensorFragment::OnSaveInstanceState(Android.OS.Bundle)' in assembly: 'BatchStepSensor.dll' ---> Mono.Cecil.ResolutionException: Failed to resolve System.Void Android.OS.BaseBundle::PutInt(System.String,System.Int32)
    at Mono.Linker.Steps.MarkStep.HandleUnresolvedMethod(MethodReference reference)
    at Mono.Linker.Steps.MarkStep.MarkMethod(MethodReference reference)
    at Mono.Linker.Steps.MarkStep.MarkInstruction(Instruction instruction)
    at Mono.Linker.Steps.MarkStep.MarkMethodBody(MethodBody body)
    at Mono.Linker.Steps.MarkStep.ProcessMethod(MethodDefinition method)
    at Mono.Linker.Steps.MarkStep.ProcessQueue()
    --- End of inner exception stack trace ---
    at Mono.Linker.Steps.MarkStep.ProcessQueue()
    at Mono.Linker.Steps.MarkStep.ProcessPrimaryQueue()
    at Mono.Linker.Steps.MarkStep.Process()
    at MonoDroid.Tuner.MonoDroidMarkStep.Process(LinkContext context)
    at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
    at Mono.Linker.Pipeline.Process(LinkContext context)
    at MonoDroid.Tuner.Linker.Process(LinkerOptions options, ILogger logger, LinkContext& context)
    at Xamarin.Android.Tasks.LinkAssemblies.Execute(DirectoryAssemblyResolver res)
    at Xamarin.Android.Tasks.LinkAssemblies.Execute()
    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
    at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()

Which is due to an outdated Mono.Android.dll in the obj directory:

Target Name=_CopyIntermediateAssemblies
    CopyIfChanged
    ...
    Skipping C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v9.0\Mono.Android.dll its up to date
    ...
    ModifiedFiles:
        obj\Release\linksrc\BatchStepSensor.dll

Reviewing the <CopyIfChanged/> MSBuild task, there is a code path
where the copy is skipped if the source file is newer than the
destination.

In the case of modifying $(TargetFrameworkVersion) on an incremental
build, a different Mono.Android.dll would have an older timestamp.
The copy would be skipped.

A fix here is to also check the size of the source and destination
files, and only allow the copy to be skipped if the sizes match.

_ConvertResourcesCases

These changes surfaced a bug in the _ConvertResourcesCases target,
if build.props changes then:

  • The _CompileResources target runs
  • _ConvertResourcesCases is skipped...

And you get a build error such as:

Resources\layout\test.axml(3): error APT0000: resource drawable/IMALLCAPS (aka UnnamedProject.UnnamedProject:drawable/IMALLCAPS) not found.

The solution here is to make the Inputs of _CompileResources and
_ConvertResourcesCases the same. I also adjusted a test to check
both aapt and aapt2.

@jonathanpeppers jonathanpeppers requested a review from dellis1972 Jul 29, 2019
namespace Xamarin.Android.Build.Tests
{
[TestFixture]
public class CopyIfChangedTests

This comment has been minimized.

Copy link
@dellis1972

dellis1972 Jul 29, 2019

Contributor

love it ❤️ 😄

@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:copyifchanged branch from b0b5e54 to 2c8eead Jul 30, 2019
Fixes: #2584

Building a project with two different `$(TargetFrameworkVersion)`:

    msbuild /p:Configuration=Release /p:TargetFrameworkVersion=v4.4
    msbuild /t:Configuration=Release /p:TargetFrameworkVersion=v9.0

Results in a build error:

    error MSB4018: The "LinkAssemblies" task failed unexpectedly.
    Mono.Linker.MarkException: Error processing method: 'System.Void BatchStepSensor.BatchStepSensorFragment::OnSaveInstanceState(Android.OS.Bundle)' in assembly: 'BatchStepSensor.dll' ---> Mono.Cecil.ResolutionException: Failed to resolve System.Void Android.OS.BaseBundle::PutInt(System.String,System.Int32)
        at Mono.Linker.Steps.MarkStep.HandleUnresolvedMethod(MethodReference reference)
        at Mono.Linker.Steps.MarkStep.MarkMethod(MethodReference reference)
        at Mono.Linker.Steps.MarkStep.MarkInstruction(Instruction instruction)
        at Mono.Linker.Steps.MarkStep.MarkMethodBody(MethodBody body)
        at Mono.Linker.Steps.MarkStep.ProcessMethod(MethodDefinition method)
        at Mono.Linker.Steps.MarkStep.ProcessQueue()
        --- End of inner exception stack trace ---
        at Mono.Linker.Steps.MarkStep.ProcessQueue()
        at Mono.Linker.Steps.MarkStep.ProcessPrimaryQueue()
        at Mono.Linker.Steps.MarkStep.Process()
        at MonoDroid.Tuner.MonoDroidMarkStep.Process(LinkContext context)
        at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
        at Mono.Linker.Pipeline.Process(LinkContext context)
        at MonoDroid.Tuner.Linker.Process(LinkerOptions options, ILogger logger, LinkContext& context)
        at Xamarin.Android.Tasks.LinkAssemblies.Execute(DirectoryAssemblyResolver res)
        at Xamarin.Android.Tasks.LinkAssemblies.Execute()
        at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
        at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()

Which is due to an outdated `Mono.Android.dll` in the `obj` directory:

    Target Name=_CopyIntermediateAssemblies
        CopyIfChanged
        ...
        Skipping C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v9.0\Mono.Android.dll its up to date
        ...
        ModifiedFiles:
            obj\Release\linksrc\BatchStepSensor.dll

Reviewing the `<CopyIfChanged/>` MSBuild task, there is a code path
where the copy is skipped if the source file is newer than the
destination.

In the case of modifying `$(TargetFrameworkVersion)` on an incremental
build, a *different* `Mono.Android.dll` would have an older timestamp.
The copy would be skipped.

A fix here is to *also* check the size of the source and destination
files, and only allow the copy to be skipped if the sizes match.

~~ _ConvertResourcesCases ~~

These changes surfaced a bug in the `_ConvertResourcesCases` target`,
if `build.props` changes then:

* The `_CompileResources` target runs
* `_ConvertResourcesCases` is skipped...

And you get a build error such as:

    Resources\layout\test.axml(3): error APT0000: resource drawable/IMALLCAPS (aka UnnamedProject.UnnamedProject:drawable/IMALLCAPS) not found.

The solution here is to make the `Inputs` of `_CompileResources` and
`_ConvertResourcesCases` the same. I also adjusted a test to check
both aapt and aapt2.
@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:copyifchanged branch from 0e0a818 to b632e9a Jul 30, 2019
@jonathanpeppers jonathanpeppers requested a review from jonpryor as a code owner Jul 30, 2019
.gitignore Outdated Show resolved Hide resolved
@jonpryor jonpryor merged commit 7836efb into xamarin:master Aug 1, 2019
2 of 5 checks passed
2 of 5 checks passed
Xamarin.Android Build #merge-c7b0f6caaa74c991f2f888cc7954793fadee53b2-1 failed
Details
Ubuntu PR Release Pipeline Build Build started for merge commit.
Details
macOS PR Release Pipeline Build Build started for merge commit.
Details
Xamarin.Android (Prepare bundle) Prepare bundle succeeded
Details
license/cla All CLA requirements met.
Details
@jonathanpeppers jonathanpeppers deleted the jonathanpeppers:copyifchanged branch Aug 5, 2019
jonpryor added a commit that referenced this pull request Aug 9, 2019
Fixes: #2584

Building a project with two different `$(TargetFrameworkVersion)`:

	msbuild /p:Configuration=Release /p:TargetFrameworkVersion=v4.4
	msbuild /t:Configuration=Release /p:TargetFrameworkVersion=v9.0

Results in a build error:

	error MSB4018: The "LinkAssemblies" task failed unexpectedly.
	Mono.Linker.MarkException: Error processing method: 'System.Void BatchStepSensor.BatchStepSensorFragment::OnSaveInstanceState(Android.OS.Bundle)' in assembly: 'BatchStepSensor.dll' ---> Mono.Cecil.ResolutionException: Failed to resolve System.Void Android.OS.BaseBundle::PutInt(System.String,System.Int32)
	    at Mono.Linker.Steps.MarkStep.HandleUnresolvedMethod(MethodReference reference)
	    at Mono.Linker.Steps.MarkStep.MarkMethod(MethodReference reference)
	    at Mono.Linker.Steps.MarkStep.MarkInstruction(Instruction instruction)
	    at Mono.Linker.Steps.MarkStep.MarkMethodBody(MethodBody body)
	    at Mono.Linker.Steps.MarkStep.ProcessMethod(MethodDefinition method)
	    at Mono.Linker.Steps.MarkStep.ProcessQueue()
	    --- End of inner exception stack trace ---
	    at Mono.Linker.Steps.MarkStep.ProcessQueue()
	    at Mono.Linker.Steps.MarkStep.ProcessPrimaryQueue()
	    at Mono.Linker.Steps.MarkStep.Process()
	    at MonoDroid.Tuner.MonoDroidMarkStep.Process(LinkContext context)
	    at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
	    at Mono.Linker.Pipeline.Process(LinkContext context)
	    at MonoDroid.Tuner.Linker.Process(LinkerOptions options, ILogger logger, LinkContext& context)
	    at Xamarin.Android.Tasks.LinkAssemblies.Execute(DirectoryAssemblyResolver res)
	    at Xamarin.Android.Tasks.LinkAssemblies.Execute()
	    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
	    at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()

Which is due to an outdated `Mono.Android.dll` in the `obj` directory:

	Target Name=_CopyIntermediateAssemblies
	    CopyIfChanged
	    ...
	    Skipping C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v9.0\Mono.Android.dll its up to date
	    ...
	    ModifiedFiles:
	        obj\Release\linksrc\BatchStepSensor.dll

Reviewing the `<CopyIfChanged/>` MSBuild task, there is a code path
where the copy is skipped if the source file is newer than the
destination.

In the case of modifying `$(TargetFrameworkVersion)` on an
incremental build, a *different* `Mono.Android.dll` would have an
older timestamp.  The copy would be skipped.

A fix here is to *also* check the size of the source and destination
files, and only allow the copy to be skipped if the sizes match.

~~ _ConvertResourcesCases ~~

These changes surfaced a bug in the `_ConvertResourcesCases` target`;
if `build.props` changes then:

  * The `_CompileResources` target runs
  * The `_ConvertResourcesCases` target is skipped (!)

And you get a build error such as:

	Resources\layout\test.axml(3): error APT0000: resource drawable/IMALLCAPS (aka UnnamedProject.UnnamedProject:drawable/IMALLCAPS) not found.

The solution here is to make the `Inputs` of `_CompileResources` and
`_ConvertResourcesCases` the same.  I also adjusted a test to check
both `aapt` and `aapt2`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.