Update ExcelDna.Integration NuGet Package to support the new PackageReference format

[augustoproiete]

Spin-off of #138 to discuss how we want to support the PackageReference format in the ExcelDna.Integration NuGet package.

Background

• When a user installs the ExcelDna.AddIn NuGet package it comes with XLL templates that includes the ExcelDna.Integration assembly embedded in it
• The ExcelDna.AddIn NuGet package NuGet package also depends on the ExcelDna.Integration NuGet package, which brings a copy of the ExcelDna.Integration assembly and references it in the project - necessary for developing add-ins
• Currently ExcelDna.Integration relies on a PowerShell script that sets the reference to ExcelDna.Integration to be CopyLocal=false to make sure this assembly doesn't get copied to the $(TargetDir) because we want to use the version that's embedded in the XLL template that comes with ExcelDna.AddIn - and historically it caused problems to have ExcelDna.Integration.dll in the same folder - this may be something to revisit in a different issue in the future, but for the purposes of this issue let's keep the same behavior we have today which is make sure we don't have a copy of ExcelDna.Integration.dll in the output folder of an Excel Add-In

Known issues

• The PackageReference format does not support the install.ps1 PowerShell script that runs during installation of the package
• Developers often need to reference the ExcelDna.Integration assembly in other projects, such as Unit Test projects, and in those cases they actually need a copy of ExcelDna.Integration.dll in the output folder of their unit test project

Proposed solution

I believe the CopyLocal=false behavior should be a concern of the ExcelDna.AddIn package instead, and not a concern of ExcelDna.Integration.

As part of adding support for PackageReference format, we should remove the install.ps1 from the ExcelDna.Integration and projects that add this NuGet package to their projects will get a regular reference to ExcelDna.Integration as expected (which fixes the issue for unit test projects, etc.).

We'll let the ExcelDna.AddIn NuGet package handle enforce the CopyLocal=false behavior itself.

Thoughts?

/cc @govert @screig

[govert]

In this context, we might investigate reference assemblies too: dotnet/roslyn#20418

[govert]

@augustoproiete So is your suggestion that ExcelDna.Integration not adjust the Private tag, and that the ExcelDna.AddIn build task will delete the ExcelDna.Integration file?

I'm still not even sure this is worth bothering with...

[govert]

Here's a bit of info on the Private tag (according to the StackOverflow where I'm copying from, "the comment in Microsoft.Common.CurrentVersion.targets:1620 where it is applied"):

 <!--
    ============================================================

                                        ResolveAssemblyReferences

    Given the list of assemblies, find the closure of all assemblies that they depend on. These are
    what we need to copy to the output directory.

        [IN]
        @(Reference) - List of assembly references as fusion names.
        @(_ResolvedProjectReferencePaths) - List of project references produced by projects that this project depends on.

            The 'Private' attribute on the reference corresponds to the Copy Local flag in IDE.
            The 'Private' flag can have three possible values:
                - 'True' means the reference should be Copied Local
                - 'False' means the reference should not be Copied Local
                - [Missing] means this task will decide whether to treat this reference as CopyLocal or not.

        [OUT]
        @(ReferencePath) - Paths to resolved primary files.
        @(ReferenceDependencyPaths) - Paths to resolved dependency files.
        @(_ReferenceRelatedPaths) - Paths to .xmls and .pdbs.
        @(ReferenceSatellitePaths) - Paths to satellites.
        @(_ReferenceSerializationAssemblyPaths) - Paths to XML serialization assemblies created by sgen.
        @(_ReferenceScatterPaths) - Paths to scatter files.
        @(ReferenceCopyLocalPaths) - Paths to files that should be copied to the local directory.
    ============================================================
    -->

[augustoproiete]

@augustoproiete So is your suggestion that ExcelDna.Integration not adjust the Private tag, and that the ExcelDna.AddIn build task will delete the ExcelDna.Integration file?

Yes on not adjusting the private tag after we install ExcelDna.Integration so that it behaves like a regular NuGet package and does what you'd expect. No (hopefully) on deleting the file from the file system. I believe I can update our .targets file to look into @(ReferenceCopyLocalPaths) and remove the ExcelDna.Integration file from that list, before MSBuild even copies the file to the output directory. That means we won't really care anymore if the user has Copy Local=false or Copy Local=true as we'll be always overriding this to get the behavior we want.

That also improves the experience of developers who need to reference this package when not creating an Excel Add-In. As I mentioned above, when creating Unit Test projects for Excel-DNA add-ins, developers need to reference this DLL because of IExcelAddIn, ExcelRibbon, etc. and it's amazing how many times I had developers approach me at my company when they can't figure out why their project compiles successfully but running the unit tests fail to start. I doubt that only happens in my company.

The Copy Local=false is an internal Excel-DNA quirk IMO that we should hide from developers if we can...

[govert]

I agree - it would be good to remove the Install.ps1 from ExcelDna.Integration if we there is a way to do what you suggest in the ExcelDna.AddIn build step. I don't know enough about the msbuild passes to know how much we can interfere with the build process.

[augustoproiete]

FYI @govert pointed me to this discussion in the Visual Studio Developer Community which is about the need for restarting Visual Studio after making changes to .props files (which affects Excel-DNA add-ins), and this issue has been fixed in the new .csproj project system (which uses PackageReference) and it doesn't seem it will ever get fixed in the old .csproj project system...

[govert]

I think the plan for ExcelDna.Integration.dll is to allow it to be in the output directory i.e. we stop settings CopyLocal=false and we also don't fiddle with it in the build task.

• If there are any issues with having the correct version of ExcelDna.Integration.dll (i.e. the one matching the ExcelDna.Loader.dll inside the running .xll) then we consider that a bug.
• If there are mismatched versions of ExcelDna.Integration.dll compared to the embedded ExcelDna.Loader.dll, then we expect the add-in to fail fast at load-time.

There is some evidence that I used to think having the correct version ExcelDna.Integration.dll in the output directory does not cause trouble: https://groups.google.com/forum/#!searchin/exceldna/copylocal%7Csort:date/exceldna/XOC3Phm0nWs/fAVshKVMTk4J