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

Add test for Binding Projects Incremental Build #8706

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

dellis1972
Copy link
Contributor

@dellis1972 dellis1972 commented Feb 7, 2024

Fixes #8658
Fixes #8698

The PR fixes an issue where the bindings files in intermediate generated folder get deleted on subsequent builds. This causes the entire binding process to run again, slowing down builds.
The root fo the problem is the _ClearGeneratedManagedBindings target. It was designed to clean out the generated folder in the case where no binding libraries were present. However it turns out if was running during a design time build! During design time builds the binding library item groups are not evaludated, so the _ClearGeneratedManagedBindings would run.. deleting everything.

To fix this issue we only run the _ClearGeneratedManagedBindings in a standard build. The good thing is this allowed us time to take a look at our incremental build process for bindings. The binding generator does produce a projitems file which lists all the files it generated. We can use that data to incrementally clean up the generated folder of old files. This way code that is no longer needed can be removed. While we know the linker will eventually remove unused classes and types, it is better to not have to compile this stuff in the first place.

@dellis1972 dellis1972 force-pushed the Issue8658 branch 2 times, most recently from 31a45ed to d03b942 Compare March 25, 2024 11:03
Fixes dotnet#8658
Fixes dotnet#8698

The PR fixes an issue where the bindings files in intermediate
`generated` folder get deleted on subsequent builds. This causes
the entire binding process to run again, slowing down builds.

The root fo the problem is the `_ClearGeneratedManagedBindings`
target. It was designed to clean out the `generated` folder in
the case where no binding libraries were present. However it
turns out if was running during a design time build! During
design time builds the binding library item groups are not
evaluated, so the `_ClearGeneratedManagedBindings` would run..
deleting everything.

To fix this issue we only run the `_ClearGeneratedManagedBindings`
in a standard build. The good thing is this allowed us time to
take a look at our incremental build process for bindings.
The binding generator does produce a `.projitems` file which lists
all the files it generated. We can use that data to incrementally
clean up the `generated` folder of old files. This way code that
is no longer needed can be removed. While we know the linker will
eventually remove unused classes and types, it is better to not
have to compile this stuff in the first place.
List<ITaskItem> files = new List<ITaskItem> ();
if (result && GeneratedFileListFile != null && File.Exists (GeneratedFileListFile)) {
var doc = XDocument.Load (GeneratedFileListFile);
var compileItems = doc.XPathSelectElements ("//Project/ItemGroup/Compile");
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked in my tests?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why it worked in the tests; if I apply this patch:

diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/Generator.cs b/src/Xamarin.Android.Build.Tasks/Tasks/Generator.cs
index a3ee0e211..7b58ed3f4 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/Generator.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/Generator.cs
@@ -142,11 +142,13 @@ namespace Xamarin.Android.Tasks
 			var result = base.RunTask ();
 			List<ITaskItem> files = new List<ITaskItem> ();
 			if (result && GeneratedFileListFile != null && File.Exists (GeneratedFileListFile)) {
+				Log.LogDebugMessage ($"# jonp: SHOULD process {GeneratedFileListFile}");
 				var doc = XDocument.Load (GeneratedFileListFile);
 				var compileItems = doc.XPathSelectElements ("//Project/ItemGroup/Compile");
 				foreach (var item in compileItems) {
 					var file = item.Attribute ("Include");
 					if (file != null && File.Exists (file.Value)) {
+						Log.LogDebugMessage ($"# jonp: found //Compile/@Include value: {file.Value}");
 						files.Add (new TaskItem (file.Value));
 					}
 				}

then build this branch locally, and do the equivalent of:

% dotnet new androidlib
% cat > Example.java <<EOF
package e;

public class Example {
    public static void e() {
    }
}
EOF
% dotnet build -v:diag > b.txt

Then I see:

# jonp: SHOULD process obj/Debug/net9.0-android/generated/src/gxa-8706.projitems

but I don't see # jonp: found //Compile/@Include value. I also believe that the GeneratedFiles output property is empty, as it doesn't appear in the diagnostic log.

The contents of gxa-8706.projitems:

<?xml version="1.0" encoding="utf-8"?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <DefineConstants>$(DefineConstants);ANDROID_1;ANDROID_2;ANDROID_3;ANDROID_4;ANDROID_5;ANDROID_6;ANDROID_7;ANDROID_8;ANDROID_9;ANDROID_10;ANDROID_11;ANDROID_12;ANDROID_13;ANDROID_14;ANDROID_15;ANDROID_16;ANDROID_17;ANDROID_18;ANDROID_19;ANDROID_20;ANDROID_21;ANDROID_22;ANDROID_23;ANDROID_24;ANDROID_25;ANDROID_26;ANDROID_27;ANDROID_28;ANDROID_29;ANDROID_30;ANDROID_31;ANDROID_32;ANDROID_33;ANDROID_34</DefineConstants>
  </PropertyGroup>
  <!-- Classes -->
  <ItemGroup>
    <Compile Include="$(MSBuildThisFileDirectory)E.Example.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)Java.Interop.__TypeRegistrations.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)__NamespaceMapping__.cs" />
  </ItemGroup>
  <!-- Enums -->
  <ItemGroup />
</Project>

Copy link
Member

Choose a reason for hiding this comment

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

…so I use this stackoverflow answer to add use of an XmlNamespaceManager so that the XPath query can. use namespaces, resulting in:

		void AddGeneratedFiles ()
		{
			List<ITaskItem> files = new List<ITaskItem> ();
			Log.LogDebugMessage ($"# jonp: SHOULD process {GeneratedFileListFile}");
			var doc = XDocument.Load (GeneratedFileListFile);

			var nsmgr = new XmlNamespaceManager(new NameTable());
			nsmgr.AddNamespace ("msb", "http://schemas.microsoft.com/developer/msbuild/2003");

			var compileItems = doc.XPathSelectElements ("//msb:Project/msb:ItemGroup/msb:Compile", nsmgr);
			foreach (var item in compileItems) {
				var file = item.Attribute ("Include");
				if (file != null && File.Exists (file.Value)) {
					Log.LogDebugMessage ($"# jonp: found //Compile/@Include value: {file.Value}");
					files.Add (new TaskItem (file.Value));
				}
			}
			GeneratedFiles = files.ToArray ();
		}

and realize, to my horror, that it still doesn't work, because of the File.Exists() check: the value of file.Attribute("Include").Value will be $(MSBuildThisFileDirectory)E.Example.cs, which will not exist. It will (almost certainly) never exist, because a directory literally named $(MSBuildThisFileDirectory) will (almost certainly) never exist. (Unless you're bananas and do mkdir '$(MSBuildThisFileDirectory)', but who would do such a thing?)

This in turn means that, as-is, @(_GeneratedBindingFiles) will always be empty, which suggests that most of the changes to the GenerateBindings target are in fact dead code.

Copy link
Member

Choose a reason for hiding this comment

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

…which is a long-winded way of explaining that I think this doesn't work, and is effectively dead (unexecuted) code.

I'm trying to remove it; let's see if anything breaks.

@jonpryor
Copy link
Member

Draft commit message:

Fixes: https://github.com/xamarin/xamarin-android/issues/8658
Fixes: https://github.com/xamarin/xamarin-android/issues/8698

Design-time builds don't play nicely with binding project builds:

	% dotnet new androidlib
	% cat > Example.java <<EOF
	package e;

	public class Example {
	    public static void e() {
	    }
	}
	EOF
	% dotnet build -p:DesignTimeBuild=true -v:diag

After this initial Design-Time build, we have the following generated
source code for the binding:

	% find obj -iname \*.cs | xargs ls -l
	-rw-r--r--  1 user staff   197 Mar 25 19:22 obj/Debug/net8.0-android/.NETCoreApp,Version=v8.0.AssemblyAttributes.cs
	-rw-r--r--  1 user staff   441 Mar 25 19:22 obj/Debug/net8.0-android/__Microsoft.Android.Resource.Designer.cs
	-rw-r--r--  1 user staff  2975 Mar 25 19:22 obj/Debug/net8.0-android/generated/src/E.Example.cs
	-rw-r--r--  1 user staff  1518 Mar 25 19:22 obj/Debug/net8.0-android/generated/src/Java.Interop.__TypeRegistrations.cs
	-rw-r--r--  1 user staff   696 Mar 25 19:22 obj/Debug/net8.0-android/generated/src/__NamespaceMapping__.cs
	-rw-r--r--  1 user staff  1094 Mar 25 19:22 obj/Debug/net8.0-android/gxa-8706.AssemblyInfo.cs
	-rw-r--r--  1 user staff   407 Mar 25 19:22 obj/Debug/net8.0-android/gxa-8706.GlobalUsings.g.cs

Run a Design-Time build *again*:

	% dotnet build -p:DesignTimeBuild=true -v:diag

…and we're now missing files (?!):

	% find obj -iname \*.cs | xargs ls -l                     
	-rw-r--r--  1 user staff   197 Mar 25 19:22 obj/Debug/net8.0-android/.NETCoreApp,Version=v8.0.AssemblyAttributes.cs
	-rw-r--r--  1 user staff   441 Mar 25 19:22 obj/Debug/net8.0-android/__Microsoft.Android.Resource.Designer.cs
	-rw-r--r--  1 user staff  1094 Mar 25 19:22 obj/Debug/net8.0-android/gxa-8706.AssemblyInfo.cs
	-rw-r--r--  1 user staff   407 Mar 25 19:22 obj/Debug/net8.0-android/gxa-8706.GlobalUsings.g.cs

In particular, `$(IntermediateOutputPath)generated/*/**.cs` is gone,
including `E.Example.cs`!

The result of this is that Design-Time builds and "normal" builds
"fight" each other, constantly generating and deleting files, slowing
down incremental builds.

The root of the problem is the `_ClearGeneratedManagedBindings`
target: It was designed to clean out the `generated` folder in the
case where no binding libraries were present.  However, it turns out
it was running during a design time build!  During design time builds
the binding library item groups are not evaluated, so the
`_ClearGeneratedManagedBindings` target would run, deleting everything.

Fix this by ensuring we only run the `_ClearGeneratedManagedBindings`
target in in "standard"/*non*-Design-Time builds.

<<<
(jonp: TODO: remove this section?  Since the code to evaluate a
`.projitems` file doesn't appear to *work*, we can either:

 1. Remove the "dead"/non-executed code, or
 2. Leave it in but mention that it doesn't work yet.)

The good thing is this allowed us time to take a look at our
incremental build process for bindings: The binding generator does
produce a `.projitems` file which lists all the files it generated.
We can use that data to incrementally clean up the `generated` folder
of old files.  This way code that is no longer needed can be removed.
While we know the linker will eventually remove unused classes and
types, it is better to not have to compile this stuff in the first
place. 
>>>

Context: https://github.com/xamarin/xamarin-android/pull/8706/files#r1538222261

dotnet@dd1f25b
("initial" commit) had code within the `<BindingsGenerator/>` task
to parse the `generator`-emitted `.projitems` file, to read out the
`@(Compile)` group:

    List<ITaskItem> files = new List<ITaskItem> ();
    var doc = XDocument.Load (GeneratedFileListFile);
    var compileItems = doc.XPathSelectElements ("//Project/ItemGroup/Compile");
    foreach (var item in compileItems) {
        var file = item.Attribute ("Include");
        if (file != null && File.Exists (file.Value)) {
            files.Add (new TaskItem (file.Value));
        }
    }
    GeneratedFiles = files.ToArray ();

There were two fundamental problems with this code:

 1. No XML namespaces
 2. The `File.Exists()` check.

`GeneratedFileListFile` would be the `.projitems` file,
which is a normal MSBuild file with a default xmlns:

    <?xml version="1.0" encoding="utf-8"?>
    <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
      <PropertyGroup>
        <DefineConstants>$(DefineConstants);ANDROID_1;ANDROID_2;ANDROID_3;ANDROID_4;ANDROID_5;ANDROID_6;ANDROID_7;ANDROID_8;ANDROID_9;ANDROID_10;ANDROID_11;ANDROID_12;ANDROID_13;ANDROID_14;ANDROID_15;ANDROID_16;ANDROID_17;ANDROID_18;ANDROID_19;ANDROID_20;ANDROID_21;ANDROID_22;ANDROID_23;ANDROID_24;ANDROID_25;ANDROID_26;ANDROID_27;ANDROID_28;ANDROID_29;ANDROID_30;ANDROID_31;ANDROID_32;ANDROID_33;ANDROID_34</DefineConstants>
      </PropertyGroup>
      <!-- Classes -->
      <ItemGroup>
        <Compile Include="$(MSBuildThisFileDirectory)E.Example.cs" />
        <Compile Include="$(MSBuildThisFileDirectory)Java.Interop.__TypeRegistrations.cs" />
        <Compile Include="$(MSBuildThisFileDirectory)__NamespaceMapping__.cs" />
      </ItemGroup>
      <!-- Enums -->
      <ItemGroup />
    </Project>

The original `doc.XPathSelectElements()` expression didn't have any
XML namespaces present, so `compileItems` would always be *empty*.

This can be fixed by using an `XmlNamespaceManager`:

	List<ITaskItem> files = new List<ITaskItem> ();
	Log.LogDebugMessage ($"# jonp: SHOULD process {GeneratedFileListFile}");
	var doc = XDocument.Load (GeneratedFileListFile);
	var nsmgr = new XmlNamespaceManager(new NameTable());
	nsmgr.AddNamespace ("msb", "http://schemas.microsoft.com/developer/msbuild/2003");
	var compileItems = doc.XPathSelectElements ("//msb:Project/msb:ItemGroup/msb:Compile", nsmgr);
	foreach (var item in compileItems) {
		var file = item.Attribute ("Include");
		if (file != null && File.Exists (file.Value)) {
			Log.LogDebugMessage ($"# jonp: found //Compile/@include value: {file.Value}");
			files.Add (new TaskItem (file.Value));
		}
	}
	GeneratedFiles = files.ToArray ();

Now `compileItems` has elements!

Then we encounter the `File.Exists()` problem: *we* are parsing the
`.projitems` file, ***not MSBuild***, so if (when) the file contains
MSBuild-isms such as `$(MSBuildThisFileDirectory)`, those pass through
unchanged!  For example, `compileItems.ElementAt(0)` would be:

    <Compile Include="$(MSBuildThisFileDirectory)E.Example.cs" />

`item.Attribute("Include").Value` would be:

    $(MSBuildThisFileDirectory)E.Example.cs

which means we're trying to *literally* do:

    File.Exists("$(MSBuildThisFileDirectory)E.Example.cs")

No expansion is occurring.

This has (approximately) ***no*** chance of working.

At minimum  we'd have to `string.Replace()` the sub-string
`$(MSBuildThisFileDirectory)` with `OutputDirectory`.

However, the fact that all the unit tests pass means one of two things:

 1. We need more unit tests ;-), or
 2. We don't actually need to parse the `.projitems` file.

Assume that (2) is the case: update `<BindingsGenerator/>` et al to
*remove* the processing of `.projitems`.

Does It Work™?
@jonpryor jonpryor merged commit d798cc9 into dotnet:main Mar 26, 2024
47 checks passed
grendello added a commit that referenced this pull request Mar 27, 2024
* main:
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
grendello added a commit that referenced this pull request Mar 27, 2024
* main:
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
  LEGO: Merge pull request 8818
  Bump to dotnet/installer@b40c44502d 9.0.100-preview.3.24165.20 (#8817)
  Bump com.android.tools:r8 from 8.2.47 to 8.3.37 (#8816)
  [Mono.Android] Prevent NullPointerException in TranslateStackTrace (#8795)
  Localized file check-in by OneLocBuild Task (#8815)
  [Xamarin.Android.Build.Tasks] Make all assemblies RID-specific (#8478)
  Localized file check-in by OneLocBuild Task (#8813)
grendello added a commit that referenced this pull request Apr 3, 2024
* main:
  Bump to dotnet/installer@dc43d363d2 9.0.100-preview.4.24175.5 (#8828)
  [Xamarin.Android.Build.Tasks] Update to newer ILRepack which supports debug files. (#8839)
  Bump 'NuGet.*' and 'Newtonsoft.Json' NuGet versions. (#8825)
  Localized file check-in by OneLocBuild Task (#8844)
  [LayoutBindings] Fix '[Preserve]' is obsolete warnings (#8529)
  LEGO: Merge pull request 8837
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
@dellis1972 dellis1972 deleted the Issue8658 branch April 19, 2024 12:54
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants