Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

Conversation

@jassmith
Copy link

@jassmith jassmith commented May 24, 2018

Context: #2230

The main performance problem with the collection of MSBuild targets in
Xamarin.Forms.targets is they don't build incrementally. I addressed
this with XamlC using a "stamp" file; however, it is not quite so
easy to setup the same thing with XamlG.

They way "incremental" builds are setup in MSBuild, is by specifying
the Inputs and Outputs of a <Target />. MSBuild will partially
build a target when some outputs are not up to date, and skip it
entirely if they are all up to date.

The best docs I can find on MSBuild incremental builds:
https://msdn.microsoft.com/en-us/library/ms171483.aspx

Unfortunately a few things had to happen to make this work for
XamlG:

  • Define a new target _FindXamlGFiles that is invoked before XamlG
  • _FindXamlGFiles defines the _XamlGInputs and _XamlGOutputs
    <ItemGroup />'s
  • _FindXamlGFiles must also define <Compile /> and <FileWrites />,
    in case the XamlG target is skipped
  • XamlGTask now needs to get passed in a list of OutputFiles,
    since we have computed these paths ahead of time
  • XamlGTask should validate the lengths of XamlFiles and
    OutputFiles match, used error message from MSBuild proper:
    https://github.com/Microsoft/msbuild/blob/a691a44f0e515e9a03ede8df0bff22185681c8b9/src/Tasks/Copy.cs#L505

XamlG now builds incrementally!

To give some context on how much improvement we can see with build
times, consider the following command:

msbuild Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj

If you run it once, it will take a while--this change will not improve
the first build. On the second build with the exact same command, it
should be much faster.

Before this commit, the second build on my machine takes:

40.563s

After the change:

23.692s

XamlG has cascading impact on build times when it isn't built
incrementally:

  • The C# assembly always changes
  • Hence, XamlC will always run
  • Hence, GenerateJavaStubs will always run
  • Hence, javac.exe and dx.jar will always run

I am making other improvements like this in Xamarin.Android itself,
that will further improve these times, such as:
dotnet/android#1693

~~ New MSBuild Integration Tests ~~

Added some basic MSBuild testing infrastructure:

  • Tests write project files to bin/Debug/temp/TestName
  • Each test has an sdkStyle flag for testing the new project system
    versus the old one
  • [TearDown] deletes the entire directory, with a retry for
    IOException on Windows
  • Used the Microsoft.Build.Locator NuGet package for locating
    MSBuild.exe on Windows
  • These tests take 2-5 seconds each

So for example, the simplest test, BuildAProject writes to
Xamarin.Forms.Xaml.UnitTests\bin\Debug\temp\BuildAProject(False)\test.csproj:

<?xml version="1.0" encoding="utf-8"?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <Configuration>Debug</Configuration>
    <Platform>AnyCPU</Platform>
    <OutputType>Library</OutputType>
    <OutputPath>bin\Debug</OutputPath>
    <TargetFrameworkVersion>v4.7</TargetFrameworkVersion>
  </PropertyGroup>
  <ItemGroup>
    <Reference Include="mscorlib" />
    <Reference Include="System" />
    <Reference Include="Xamarin.Forms.Core.dll">
      <HintPath>..\..\Xamarin.Forms.Core.dll</HintPath>
    </Reference>
    <Reference Include="Xamarin.Forms.Xaml.dll">
      <HintPath>..\..\Xamarin.Forms.Xaml.dll</HintPath>
    </Reference>
  </ItemGroup>
  <ItemGroup>
    <Compile Include="AssemblyInfo.cs" />
  </ItemGroup>
  <Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
  <Import Project="..\..\..\..\..\.nuspec\Xamarin.Forms.targets" />
  <ItemGroup>
    <EmbeddedResource Include="MainPage.xaml" />
  </ItemGroup>
</Project>

Invokes msbuild, and checks the intermediate output for files being
generated.

Tested scenarios:

  • Build a simple project
  • Build, then build again, and make sure targets were skipped
  • Build, then clean, make sure files are gone
  • Build, with linked files
  • Design-time build
  • Call UpdateDesignTimeXaml directly
  • Build, add a new file, build again
  • Build, update timestamp on a file, build again
  • XAML file with random XML content
  • XAML file with invalid XML content
  • A general EmbeddedResource that shouldn't go through XamlG

Adding these tests found a bug! IncrementalClean was deleting
XamlC.stamp. I fixed this by using <ItemGroup />, which will be
propery evaluated even if the target is skipped.

~~ Other Changes ~~

  • FilesWrite is actually supposed to be FileWrites, see canonical
    source of how Clean works and what FileWrites is here:
    Understanding the Clean target dotnet/msbuild#2408 (comment)
  • Moved DummyBuildEngine into MSBuild directory--makes sense?
    maybe don't need to?
  • Added a XamlGDifferentInputOutputLengths test to check the error
    message
  • Expanded DummyBuildEngine so you can assert against log messages
  • Changed a setting in .Xamarin.Forms.Android.sln so the unit test
    project is built
  • My VS IDE monkeyed with a few files, and I kept any good (or
    relevant) changes: Xamarin.Forms.UnitTests.csproj,
    Xamarin.Forms.Xaml.UnitTests\app.config, etc.

There were some checks for %(TargetPath) being blank in the C# code
of XamlGTask. In that case it was using Path.GetRandomFileName,
but we can't do this if we are setting up inputs and outputs for
XamlG. I presume this is from the designer and/or design-time builds
before DependsOnTargets="PrepareResourceNames" was added. I tested
design-time builds in VS on Windows, and $(TargetPath) was set. To
be sure we don't break anything here, I exclude inputs to XamlG if
%(TargetPath) is somehow blank.

See relevant MSBuild code for %(TargetPath) here:

https://github.com/Microsoft/msbuild/blob/05151780901c38b4613b2f236ab8b091349dbe94/src/Tasks/Microsoft.Common.CurrentVersion.targets#L2822

~~ Future changes ~~

CssG needs the exact same setup, as it was patterned after XamlG.
This should probably be done in a future PR.

Description of Change

Describe your changes here.

Bugs Fixed

  • Provide links to bugs here

API Changes

List all API changes here (or just put None), example:

Added:

  • string ListView.GroupName { get; set; } //Bindable Property
  • int ListView.GroupId { get; set; } // Bindable Property
  • void ListView.Clear ();

Changed:

  • object ListView.SelectedItem => Cell ListView.SelectedItem

Behavioral Changes

Describe any non-bug related behavioral changes that may change how users app behaves when upgrading to this version of the codebase.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

Copy link
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

looks fine.

back porting the unit tests wasn't necessary

@StephaneDelcroix
Copy link
Member

I understand this is very critical, and should move fast. but as 3.0 is in -sr mode, we're likely to reach to more users by targeting to 3.1.0 and issue a new -pre

@jonathanpeppers
Copy link
Member

Hmm, it's actually good these MSBuild tests are here because they all failed...

I see something that got lost from: #2230

I'll just push changes here to fix this up.

Context: #2230

The main performance problem with the collection of MSBuild targets in
`Xamarin.Forms.targets` is they don't build incrementally. I addressed
this with `XamlC` using a "stamp" file; however, it is not quite so
easy to setup the same thing with `XamlG`.

They way "incremental" builds are setup in MSBuild, is by specifying
the `Inputs` and `Outputs` of a `<Target />`. MSBuild will partially
build a target when some outputs are not up to date, and skip it
entirely if they are all up to date.

The best docs I can find on MSBuild incremental builds:
https://msdn.microsoft.com/en-us/library/ms171483.aspx

Unfortunately a few things had to happen to make this work for
`XamlG`:
- Define a new target `_FindXamlGFiles` that is invoked before `XamlG`
- `_FindXamlGFiles` defines the `_XamlGInputs` and `_XamlGOutputs`
  `<ItemGroup />`'s
- `_FindXamlGFiles` must also define `<Compile />` and `<FileWrites />`,
  in case the `XamlG` target is skipped
- `XamlGTask` now needs to get passed in a list of `OutputFiles`,
  since we have computed these paths ahead of time
- `XamlGTask` should validate the lengths of `XamlFiles` and
  `OutputFiles` match, used error message from MSBuild proper:
  https://github.com/Microsoft/msbuild/blob/a691a44f0e515e9a03ede8df0bff22185681c8b9/src/Tasks/Copy.cs#L505

`XamlG` now builds incrementally!

To give some context on how much improvement we can see with build
times, consider the following command:

    msbuild Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj

If you run it once, it will take a while--this change will not improve
the first build. On the second build with the exact same command, it
*should* be much faster.

Before this commit, the second build on my machine takes:

    40.563s

After the change:

    23.692s

`XamlG` has cascading impact on build times when it isn't built
incrementally:
- The C# assembly always changes
- Hence, `XamlC` will always run
- Hence, `GenerateJavaStubs` will always run
- Hence, `javac.exe` and `dx.jar` will always run

I am making other improvements like this in Xamarin.Android itself,
that will further improve these times, such as:
dotnet/android#1693

~~ New MSBuild Integration Tests ~~

Added some basic MSBuild testing infrastructure:
- Tests write project files to `bin/Debug/temp/TestName`
- Each test has an `sdkStyle` flag for testing the new project system
  versus the old one
- `[TearDown]` deletes the entire directory, with a retry for
  `IOException` on Windows
- Used the `Microsoft.Build.Locator` NuGet package for locating
  `MSBuild.exe` on Windows
- These tests take 2-5 seconds each

So for example, the simplest test, `BuildAProject` writes to
`Xamarin.Forms.Xaml.UnitTests\bin\Debug\temp\BuildAProject(False)\test.csproj`:

    <?xml version="1.0" encoding="utf-8"?>
    <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
      <PropertyGroup>
        <Configuration>Debug</Configuration>
        <Platform>AnyCPU</Platform>
        <OutputType>Library</OutputType>
        <OutputPath>bin\Debug</OutputPath>
        <TargetFrameworkVersion>v4.7</TargetFrameworkVersion>
      </PropertyGroup>
      <ItemGroup>
        <Reference Include="mscorlib" />
        <Reference Include="System" />
        <Reference Include="Xamarin.Forms.Core.dll">
          <HintPath>..\..\Xamarin.Forms.Core.dll</HintPath>
        </Reference>
        <Reference Include="Xamarin.Forms.Xaml.dll">
          <HintPath>..\..\Xamarin.Forms.Xaml.dll</HintPath>
        </Reference>
      </ItemGroup>
      <ItemGroup>
        <Compile Include="AssemblyInfo.cs" />
      </ItemGroup>
      <Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
      <Import Project="..\..\..\..\..\.nuspec\Xamarin.Forms.targets" />
      <ItemGroup>
        <EmbeddedResource Include="MainPage.xaml" />
      </ItemGroup>
    </Project>

Invokes `msbuild`, and checks the intermediate output for files being
generated.

Tested scenarios:
- Build a simple project
- Build, then build again, and make sure targets were skipped
- Build, then clean, make sure files are gone
- Build, with linked files
- Design-time build
- Call `UpdateDesignTimeXaml` directly
- Build, add a new file, build again
- Build, update timestamp on a file, build again
- XAML file with random XML content
- XAML file with invalid XML content
- A general `EmbeddedResource` that shouldn't go through XamlG

Adding these tests found a bug! `IncrementalClean` was deleting
`XamlC.stamp`. I fixed this by using `<ItemGroup />`, which will be
propery evaluated even if the target is skipped.

~~ Other Changes ~~

- `FilesWrite` is actually supposed to be `FileWrites`, see canonical
  source of how `Clean` works and what `FileWrites` is here:
  dotnet/msbuild#2408 (comment)
- Moved `DummyBuildEngine` into `MSBuild` directory--makes sense?
  maybe don't need to?
- Added a `XamlGDifferentInputOutputLengths` test to check the error
  message
- Expanded `DummyBuildEngine` so you can assert against log messages
- Changed a setting in `.Xamarin.Forms.Android.sln` so the unit test
  project is built
- My VS IDE monkeyed with a few files, and I kept any *good* (or
  relevant) changes: `Xamarin.Forms.UnitTests.csproj`,
  `Xamarin.Forms.Xaml.UnitTests\app.config`, etc.

There were some checks for `%(TargetPath)` being blank in the C# code
of `XamlGTask`. In that case it was using `Path.GetRandomFileName`,
but we can't do this if we are setting up inputs and outputs for
`XamlG`. I presume this is from the designer and/or design-time builds
before `DependsOnTargets="PrepareResourceNames"` was added. I tested
design-time builds in VS on Windows, and `$(TargetPath)` was set. To
be sure we don't break anything here, I exclude inputs to `XamlG` if
`%(TargetPath)` is somehow blank.

See relevant MSBuild code for `%(TargetPath)` here:

https://github.com/Microsoft/msbuild/blob/05151780901c38b4613b2f236ab8b091349dbe94/src/Tasks/Microsoft.Common.CurrentVersion.targets#L2822

~~ Future changes ~~

CssG needs the exact same setup, as it was patterned after `XamlG`.
This should probably be done in a future PR.
@jonathanpeppers
Copy link
Member

If you guys do fast-track this, the only thing I would recommend is to get some real QA using this in IDEs.

I recently discovered VS for Mac has some custom code in this area, and we don't want to break them if they are relying on the past "always-build" behavior: https://github.com/xamarin/md-addins/blob/master/Xamarin.Forms.Addin/Xamarin.Forms.Addin/CSharpProjector.cs

@jassmith
Copy link
Author

It will be put out as a -pre for the SR first (odd I know) so we have some decent testing time

@dansiegel
Copy link
Contributor

@jassmith not weird at all... it doesn't matter how many bugs get fixed, or how many "cool" new features are added if nobody can build, or if there are poor IDE experiences. I know I've been seeing some strange new behavior in VS Mac 7.5 when it comes to intellisense with generated files (generally speaking not just specific to XF).

@jonathanpeppers
Copy link
Member

Weird, why are the paths different when these tests run:

2018-05-24T19:27:15.0826374Z C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\MSBuild.exe /nologo /bl /t:Build /v:minimal D:\a\1\b\win_build\Xamarin.Forms.Xaml.UnitTests\bin\Debug\temp\TargetsShouldSkip(False)\test.csproj
2018-05-24T19:27:16.6567267Z D:\a\1\b\win_build\Xamarin.Forms.Xaml.UnitTests\bin\Debug\temp\TargetsShouldSkip(False)\test.csproj(24,3): error MSB4019: The imported project "D:\a\1\b\win_build\.nuspec\Xamarin.Forms.targets" was not found. Confirm that the path in the <Import> declaration is correct, and that the file exists on disk.
2018-05-24T19:27:17.0533937Z Failed   TargetsShouldSkip(False)
2018-05-24T19:27:17.0535601Z Error Message:
2018-05-24T19:27:17.0536036Z    MSBuild exited with 1
2018-05-24T19:27:17.0536407Z   Expected: 0
2018-05-24T19:27:17.0536733Z   But was:  1

It looks like the unit tests have been copied to a new directory. @rmarinho ?

I can make them check two places for Xamarin.Forms.targets if we want this green.

@jonathanpeppers
Copy link
Member

jonathanpeppers commented May 24, 2018

Yeah the way the build phase is running on VSTS, it's breaking these tests:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\MSBuild.exe /nologo /bl /t:Build /v:minimal D:\a\1\b\win_build\Xamarin.Forms.Xaml.UnitTests\bin\Release\temp\RandomXml(True)\test.csproj
D:\a\1\s\.nuspec\Xamarin.Forms.targets(36,3): error MSB4062: The "Xamarin.Forms.Build.Tasks.GetTasksAbi" task could not be loaded from the assembly D:\a\1\s\.nuspec\Xamarin.Forms.Build.Tasks.dll. Could not load file or assembly 'file:///D:\a\1\s\.nuspec\Xamarin.Forms.Build.Tasks.dll' or one of its dependencies. The system cannot find the file specified. Confirm that the <UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [D:\a\1\b\win_build\Xamarin.Forms.Xaml.UnitTests\bin\Release\temp\RandomXml(True)\test.csproj]
Failed   RandomXml(True)
Error Message:
   MSBuild exited with 1
  Expected: 0
  But was:  1

My change found the .nuspec/Xamarin.Forms.targets, but then MSBuild couldn't load an assembly.

I guess the build process is different on a release branch?

Let me know what you guys think I should do. I can make these Assert.Ignore instead of Assert.Fail, or whichever we think is best.

Context: https://devdiv.visualstudio.com/DevDiv/_build?buildId=1717939
Context: https://devdiv.visualstudio.com/DevDiv/_build?buildId=1718306

It looks like the VSTS builds for release branches are running tests
in a staging directory. This means we can't reliably import
`Xamarin.Forms.targets` as what was working locally in the
Xamarin.Forms source tree.

So to fix this:
- Look for `.nuspec/Xamarin.Forms.targets`, at the default location
  and then using the `BUILD_SOURCESDIRECTORY` environment variable as
  a fallback
- Copy all `*.targets` files to the test directory
- Our `*.csproj` files under test can import the file from there.

We have to copy the targets files here to be sure that MSBuild can
load `Xamarin.Forms.Build.Tasks.dll`, which is also referenced by the
unit tests.

I also made the tests abort earlier if they can't find
`Xamarin.Forms.targets`.
@jonathanpeppers
Copy link
Member

Fixed it ^^

If we are fine with this, I should cherry-pick b546e0a to #2755 after we merge this.

@rmarinho rmarinho merged commit b84b35b into 3.0.0 May 25, 2018
@StephaneDelcroix
Copy link
Member

This has an annoying side effect: xaml.g.cs files aren't regenerated on (XF) version update. Part of the current behavior was in place to avoid telling our users clean their bin/ and obj/ folders.

so, this should:

  1. bump the Build.Tasks version
  2. somehow write won the version in a .stamp file, and if the version differs from the current, force rerunning XamlG, and XamlC

@StephaneDelcroix
Copy link
Member

also, the MSbuild unit tests are taking 1min and 27sec to run on this machine

@StephaneDelcroix StephaneDelcroix deleted the xamlg-incremental branch May 29, 2018 09:41
@jonathanpeppers
Copy link
Member

Sounds like the _ValidateXFTasks target should write _XFTasksExpectedAbi to a Xamarin.Forms.version file in $(IntermediateOutputPath) (only if the version found in the file differs). Then all the other targets add this file as input so if the timestamp changes, they run again.

As far as the tests go, they are going to be a bit slow due to their nature. Xamarin.Android's tests like this take 30 seconds to 1 min each... We could drop the sdkStyle parameter on all these tests, and change it to a const to be switched on later if needed. This would 2x the test time. I only tested SDK-style projects because I was afraid of breaking something.

Do you want me to add these to #2755 or a future PR? Maybe just speed up the tests and the rest can be addressed in another PR?

@samhouts samhouts added this to the 3.0.0 milestone Jun 1, 2018
@StephaneDelcroix StephaneDelcroix mentioned this pull request Jun 5, 2018
4 tasks
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.

7 participants