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

Commits on Mar 25, 2024

  1. Add test for Binding Projects

    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.
    dellis1972 committed Mar 25, 2024
    Configuration menu
    Copy the full SHA
    dd1f25b View commit details
    Browse the repository at this point in the history
  2. Fix indentation

    jonpryor committed Mar 25, 2024
    Configuration menu
    Copy the full SHA
    8af4244 View commit details
    Browse the repository at this point in the history

Commits on Mar 26, 2024

  1. Remove dead code.

    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 committed Mar 26, 2024
    Configuration menu
    Copy the full SHA
    10e3e66 View commit details
    Browse the repository at this point in the history