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] minimal Reference Assemblies support #2474

Merged
merged 1 commit into from Dec 5, 2018

Conversation

Projects
None yet
3 participants
@jonathanpeppers
Contributor

jonathanpeppers commented Nov 30, 2018

This is the beginning of our support for MSBuild/Roslyn "Reference
Assemblies".

Adding the following to a csproj file:

<ProduceReferenceAssembly>True</ProduceReferenceAssembly>

Causes a bin\Debug\ref\MyLibrary.dll to exist alongside
bin\Debug\MyLibrary.dll.

Two item groups, @(ReferenceCopyLocalPaths) and @(ReferencePath)
will have new %(ReferenceAssembly) metadata:

bin\Debug\MyLibrary.dll
    ReferenceAssembly = C:\full\path\to\bin\Debug\ref\MyLibrary.dll

If an assembly does not include a reference assembly, it will still
maintain %(ReferenceAssembly) metadata pointing to itself:

bin\Debug\OtherLibrary.dll
    ReferenceAssembly = C:\full\path\to\bin\Debug\OtherLibrary.dll

In some cases this metadata might not be there, such as using a path
as input to ResolveAssemblies ($(OutDir)$(TargetFileName)), so our
ResolveAssemblies MSBuild task should produce the value if it does
not exist.

Changes

The biggest change here is to rework ResolveAssemblies so that it
preserves item metadata. The metadata was getting lost in various
ways:

  • Use of %(ReferencePath.Identity) instead of @(ReferencePath).
  • Any new TaskItem instances created should use the ctor taking in
    an existing ITaskItem. This preserves the metadata from the
    original item.

We also needed to make some changes to support %(ReferenceAssembly):

  • New TaskItem objects created by a file path should set
    %(ReferenceAssembly).
  • We should check if %(ReferenceAssembly) is missing, and set it
    where appropriate.

I looked into reworking _GenerateJavaStubs so it can be skipped
when a reference assembly didn't change.

I merely changed a single input: @(_ResolvedAssemblies) to
@(_ResolvedAssemblies->'%(ReferenceAssembly)').

Unfortunately, this would break! A developer could write this
completely valid C# class:

internal class Example : Java.Lang.Object { }

However, since we have preserved item metadata, we can now use
%(ReferenceAssembly) throughout the build. We can take advantage of
it in the future.

I added some documentation about what I found out with MSBuild
"Reference Assembly" support, in general. I also added a unit test
verifying which targets skip when $(ProduceReferenceAssembly) is set
in a library project.

I also reworked our Xamarin.Forms integration project so that it is
netstandard instead of a shared project.

This will help us test $(ProduceReference) assembly, and also has a
much simpler project file!

Other changes:

  • I let VS Windows fixup the Xamarin.Android-Tests.sln file. It
    looks like there were a few mismatched guids and other things.
  • We did not have XamlC enabled:
    [assembly: XamlCompilation (XamlCompilationOptions.Compile)]
    this will greatly improve startup times!
  • We will need to call /t:Restore on the Xamarin.Forms netstandard
    project now.

@jonathanpeppers jonathanpeppers requested review from jonpryor and dellis1972 Nov 30, 2018

to be rebuilt *less often*.
[msbuild_refassemblies]: https://github.com/dotnet/roslyn/blob/master/docs/features/refout.md

This comment has been minimized.

@jonathanpeppers

jonathanpeppers Nov 30, 2018

Contributor

The above section was the relevant good stuff from here, written by @jonpryor : https://github.com/xamarin/xamarin-android/wiki/Build-Performance-Ideas#embrace-reference-assemblies

@jonpryor

This comment has been minimized.

Contributor

jonpryor commented Nov 30, 2018

Should this also set $(ProduceReferenceAssembly)=True in e.g. tests/CodeGen-Binding//Xamarin.Android.LibraryProjectZip-LibBinding/Xamarin.Android.LibraryProjectZip-LibBinding.csproj?

@jonathanpeppers

This comment has been minimized.

Contributor

jonathanpeppers commented Nov 30, 2018

Hmm, that project would be good, but I don’t think anything would test an incremental build on it...

I think it might actually make more sense to convert the Xamarin.Forms integration project to use NetStandard instead of a shared project. Then enable it there.

@radekdoulik would that be ok with you? Would that hurt our plots?

@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:referenceassemblies-babysteps branch from f053ff8 to 72f065e Dec 3, 2018

@jonathanpeppers

This comment has been minimized.

Contributor

jonathanpeppers commented Dec 4, 2018

Well looks like calling /t:Restore directly isn't a good idea on macOS?

/Library/Frameworks/Mono.framework/Versions/5.16.0/lib/mono/msbuild/15.0/bin/MSBuild.dll /binaryLogger:/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android//bin/BuildRelease/msbuild-20181204T120645-prepare-external.binlog /p:Configuration=Release /p:AutoProvision=True /p:AutoProvisionUsesSudo=True /p:IgnoreMaxMonoVersion=False /p:Configuration=Release /p:AutoProvision=True /p:AutoProvisionUsesSudo=True /p:IgnoreMaxMonoVersion=False /t:Restore /v:normal Xamarin.Android-Tests.sln
...
TestRunner.xUnit.csproj : error MSB4057: The target "Restore" does not exist in the project.

I'm not sure why, it seemed to work for me locally?

@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:referenceassemblies-babysteps branch 2 times, most recently from 3e04390 to 89558df Dec 4, 2018

@jonathanpeppers jonathanpeppers requested a review from radekdoulik as a code owner Dec 5, 2018

Show resolved Hide resolved Makefile Outdated

@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:referenceassemblies-babysteps branch 3 times, most recently from 12b3310 to c382404 Dec 5, 2018

@jonathanpeppers

This comment has been minimized.

Contributor

jonathanpeppers commented Dec 5, 2018

Windows is green:

assets/subfolder/asset2.txt should be in the apk.

This is fixed in #2480

@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:referenceassemblies-babysteps branch from c382404 to ad7d1c8 Dec 5, 2018

@jonpryor

This comment has been minimized.

Contributor

jonpryor commented Dec 5, 2018

Suggested Squash-and-merge commit message:

[Xamarin.Android.Build.Tasks] Start Reference Assemblies support (#2474)

This is the beginning of our support for MSBuild/Roslyn
"Reference Assemblies".

Adding the following to a `.csproj` file:

	<ProduceReferenceAssembly>True</ProduceReferenceAssembly>

Causes a `bin\Debug\ref\MyLibrary.dll` to be built alongside
`bin\Debug\MyLibrary.dll`.  The new `bin\Debug\ref\MyLibrary.dll`
file is *only* changed whenever *`public` and `protected`* API in
`MyLibrary.dll` is changed.

MSBuild creates two item groups, `@(ReferenceCopyLocalPaths)` and
`@(ReferencePath)`, which have `%(ReferenceAssembly)` metadata:

	bin\Debug\MyLibrary.dll
	    ReferenceAssembly = C:\full\path\to\bin\Debug\ref\MyLibrary.dll

If an assembly does *not* include a reference assembly, it will still
maintain `%(ReferenceAssembly)` metadata pointing to itself:

	bin\Debug\OtherLibrary.dll
	    ReferenceAssembly = C:\full\path\to\bin\Debug\OtherLibrary.dll

In some cases this metadata might not be there, such as using a path
as input to `<ResolveAssemblies/>` (via `$(OutDir)$(TargetFileName)`),
so our `<ResolveAssemblies/>` task will produce the value if it does
not exist.

~~ Changes ~~

The biggest change here is to rework `<ResolveAssemblies/>` so that
it preserves item metadata.  The metadata was getting lost in
various ways:

  - Use of `%(ReferencePath.Identity)` instead of `@(ReferencePath)`.
  - Any new `TaskItem` instances created should use the ctor taking
    in an existing `ITaskItem`.  This preserves the metadata from the
    original item.

We also needed to make some changes to support `%(ReferenceAssembly)`:

  - New `TaskItem` objects created by a file path should set
    `%(ReferenceAssembly)`.
  - We should check if `%(ReferenceAssembly)` is missing, and set it
    where appropriate.

I added some documentation about what I found out with MSBuild
"Reference Assembly" support, in general.  I also added a unit test
verifying which targets skip when `$(ProduceReferenceAssembly)` is
set in a library project.

To help test Reference Assembly support,
`tests/Xamarin.Forms-Performance-Integration` has been updated so a
.NET Standard 2 project is used instead of a Shared Project.

This will help us test `$(ProduceReferenceAssembly)`, and also has a
much simpler project file!

Other changes:

  - I let VS Windows fixup the `Xamarin.Android-Tests.sln file`.
    It looks like there were a few mismatched GUIDs and other things.
  - We did not have `<XamlC/>` enabled:
    `[assembly: XamlCompilation (XamlCompilationOptions.Compile)]`
    should greatly improve startup times!
[Xamarin.Android.Build.Tasks] minimal Reference Assemblies support
This is the beginning of our support for MSBuild/Roslyn "Reference
Assemblies".

Adding the following to a `csproj` file:

    <ProduceReferenceAssembly>True</ProduceReferenceAssembly>

Causes a `bin\Debug\ref\MyLibrary.dll` to exist alongside
`bin\Debug\MyLibrary.dll`.

Two item groups, `@(ReferenceCopyLocalPaths)` and `@(ReferencePath)`
will have new `%(ReferenceAssembly)` metadata:

    bin\Debug\MyLibrary.dll
        ReferenceAssembly = C:\full\path\to\bin\Debug\ref\MyLibrary.dll

If an assembly does *not* include a reference assembly, it will still
maintain `%(ReferenceAssembly)` metadata pointing to itself:

    bin\Debug\OtherLibrary.dll
        ReferenceAssembly = C:\full\path\to\bin\Debug\OtherLibrary.dll

In some cases this metadata might not be there, such as using a path
as input to `ResolveAssemblies` (`$(OutDir)$(TargetFileName)`), so our
`ResolveAssemblies` MSBuild task should produce the value if it does
not exist.

~~ Changes ~~

The biggest change here is to rework `ResolveAssemblies` so that it
preserves item metadata. The metadata was getting lost in various
ways:

- Use of `%(ReferencePath.Identity)` instead of `@(ReferencePath)`.
- Any new `TaskItem` instances created should use the ctor taking in
  an existing `ITaskItem`. This preserves the metadata from the
  original item.

We also needed to make some changes to support `%(ReferenceAssembly)`:

- New `TaskItem` objects created by a file path should set
  `%(ReferenceAssembly)`.
- We should check if `%(ReferenceAssembly)` is missing, and set it
  where appropriate.

I looked into reworking `_GenerateJavaStubs` so it can be skipped
when a reference assembly didn't change.

I merely changed a single input: `@(_ResolvedAssemblies)` to
`@(_ResolvedAssemblies->'%(ReferenceAssembly)')`.

Unfortunately, this would break! A developer could write this
completely valid C# class:

    internal class Example : Java.Lang.Object { }

However, since we have preserved item metadata, we can now use
`%(ReferenceAssembly)` throughout the build. We can take advantage of
it in the future.

I added some documentation about what I found out with MSBuild
"Reference Assembly" support, in general. I also added a unit test
verifying which targets skip when `$(ProduceReferenceAssembly)` is set
in a library project.

I also reworked our Xamarin.Forms integration project so that it is
netstandard instead of a shared project.

This will help us test `$(ProduceReference)` assembly, and also has a
much simpler project file!

Other changes:

- I let VS Windows fixup the `Xamarin.Android-Tests.sln file`. It
  looks like there were a few mismatched guids and other things.
- We did not have XamlC enabled:
  `[assembly: XamlCompilation (XamlCompilationOptions.Compile)]`
  this will greatly improve startup times!
- We will need to call `/t:Restore` on the Xamarin.Forms netstandard
  project now.

@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:referenceassemblies-babysteps branch from ad7d1c8 to 61ccb05 Dec 5, 2018

@jonpryor jonpryor merged commit 83559e2 into xamarin:master Dec 5, 2018

2 of 3 checks passed

macOS PR Release Build Build finished. 208619 tests run, 1753 skipped, 1 failed.
Details
Ubuntu PR Build Build finished. No test results found.
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment