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

[XamlG] builds incrementally, add MSBuild integration tests #2755

Closed

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented May 17, 2018

Description of Change

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:

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.

API Changes

XamlGTask has changes, which should be an MSBuild task hidden beneath the XamlG target.

Removed:

[Output]
public ITaskItem[] GeneratedCodeFiles { get; set; }

public string OutputPath { get; set; }

Added:

[Required]
public ITaskItem[] OutputFiles { get; set; }

Behavioral Changes

XamlG builds incrementally.

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

@dellis1972
Copy link

@jonathanpeppers some useful info. Even if a target is skipped completely, if it has <ItemGroup/> values (or CreateItem I believe) those will still be evaluated.

@StephaneDelcroix
Copy link
Member

fails a unit-test

2018-05-17T14:09:36.6675299Z        (CoreCompile target) -> 
2018-05-17T14:09:36.6676332Z          XamlgFileLockTests.cs(38,5): error CS0117: 'XamlGTask' does not contain a definition for 'OutputPath' [C:\agent\_work\2\s\Xamarin.Forms.Xaml.UnitTests\Xamarin.Forms.Xaml.UnitTests.csproj]
2018-05-17T14:09:36.6677713Z          XamlgFileLockTests.cs(43,38): error CS1061: 'XamlGTask' does not contain a definition for 'GeneratedCodeFiles' and no extension method 'GeneratedCodeFiles' accepting a first argument of type 'XamlGTask' could be found (are you missing a using directive or an assembly reference?) [C:\agent\_work\2\s\Xamarin.Forms.Xaml.UnitTests\Xamarin.Forms.Xaml.UnitTests.csproj]
2018-05-17T14:09:36.6680749Z 

if (Path.DirectorySeparatorChar == '/' && outputFile.Contains(@"\"))
outputFile = outputFile.Replace('\\','/');
else if (Path.DirectorySeparatorChar == '\\' && outputFile.Contains(@"/"))
outputFile = outputFile.Replace('/', '\\');
Copy link
Member

Choose a reason for hiding this comment

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

we need to investigate if this is still an issue (#1250)

{
var binLog = Path.Combine (tempDirectory, "msbuild.binlog");
var psi = new ProcessStartInfo {
FileName = "msbuild",
Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, looks like msbuild is not in the path on the build agent so I have to "find" MSBuild on Windows.

@jonathanpeppers jonathanpeppers changed the title [XamlG] builds incrementally [XamlG] builds incrementally, add MSBuild integration tests May 18, 2018
Context: xamarin#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.
@spouliot
Copy link

This also affects XI (and likely XM even if I have not tried) as the change means a Build, without any other change, makes the cache check fail - which triggers a lot more build actions that what's needed.

@bholmes bholmes added this to the 3.2.0 milestone May 23, 2018
@migueldeicaza
Copy link
Contributor

I would like this capability to be backported to 3.0

@jassmith jassmith changed the base branch from master to 3.0.0 May 24, 2018 17:20
@jassmith jassmith changed the base branch from 3.0.0 to master May 24, 2018 17:21
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`.
@xamarin-release-manager xamarin-release-manager added the API-change Heads-up to reviewers that this PR may contain an API change label May 25, 2018
@jonathanpeppers
Copy link
Member Author

This PR got merged via #2825, we can close this one.

@samhouts samhouts removed this from the 3.2.0 milestone Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API-change Heads-up to reviewers that this PR may contain an API change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants