ExcelDna.AddIn NuGet package installation fails as a PackageReference

[govert]

This is due to install.ps1 not being supported for PackageReference. See https://groups.google.com/forum/#!topic/exceldna/GOqEBI6aiUI

[augustoproiete]

PR #147 was a step towards us not needing to run a Powershell script when installing the NuGet package, which in turn is a step towards the story of us supporting PackageReference.

I'd like to start discussing next steps.

At the moment, we do 3 different things in the install.ps1:

1. Rename an _UNINSTALLED_${newDnaFileName} to ${projectName}-AddIn.dna, if exists
2. Add a ${projectName}-AddIn.dna file, if step CommandBar by ID #1 didn't find an old file
3. Add a reference to the .targets file Update 2018-05-18: This is now done automatically by a NuGet feature implemented by Update NuGet package to use build feature for adding .targets files to projects #167

• How critical is number 1?
  Could we just drop this behavior in vNext, and leave it to the user to decide to remove .dna file(s) when uninstalling?

• Step number 2 I'm not sure is supported by NuGet without the need for Powershell. Maybe with T4? Needs more investigation... In the event that it's not supported, what about dropping this behavior in vNext, and instead add a file to the project with a static name (e.g. ExcelDnaAddIn.dna), and leave it to the users to rename it as they see fit? The build process today doesn't care about the file name being the same name as the project, so it would just work without any changes.

• Step 3 is definitely supported by NuGet (v2.5 and later). We just have to move the .targets file to a build folder, and make a couple of adjustments to the .targets file to find the relative paths of everything based on its new location.

Thoughts?

[govert]

My first thought are:

• That the new PackageReference does not support install.ps1 is a mess (not just for us), and maybe Microsoft will deal with it better in future.
• That the Excel-DNA Nuget package does not support PackageReference does not seem like a serious issue.
• I think 1 and 2 are important for making it easy to get started and making it safe to uninstall the package and re-install. In the past this was more important, since uninstall and reinstall was the easiest way to fix the debugger settings etc.

My suggestion is:

• We leave the current package operation as it is, with the install.ps1.
• We document the manual steps that should be taken to make PackageReference work - I think this is basically three steps with the new build stuff:
  • add a .dna file
  • set the reference to ExcelDna.Integration.dll to be Copy Local=false
  • set up the debugger paths.

I'd rather make it a bit more tricky (but still possible) to do the PackageReference, rather then confusing or making it harder for 'normal' users.

Just my first thoughts...

[screig]

Those three points are all valid. My personal biggest concern, and it may will be misplaced, was around the post build pack task, with the new structure that comes with csproj settings. The current post build event is currently of the form:

"$(SolutionDir)packages\ExcelDna.AddIn.0.33.9\tools\ExcelDnaPack.exe" "$(TargetDir)MyAddIn.dna" /Y

How will this work with a shared packages folder that is not under the solution folder? The dna file I could manage myself, but I have no idea how to dynamically references the shared packages folder.

[govert]

We've discussed in the past moving the packing into the build task. That might be a reasonable change that helps with this too.

[augustoproiete]

Agreed. Moving packing to the build task is the way to go. /cc @screig

As for your other comments, @govert:

That the new PackageReference does not support install.ps1 is a mess (not just for us), and maybe Microsoft will deal with it better in future.

I'm not sure they'll go back to allowing us to use install.ps1. I thought the main reasons for that were portability of packages to Linux/OSX (as Powershell was not available on these platforms then) and security (stop letting packages run arbitrary code). Maybe that story is different now that Powershell is cross-platform, but the security concern is still valid.

That the Excel-DNA Nuget package does not support PackageReference does not seem like a serious issue.

Doesn't seem like an issue these days with VS2017, but it does seem to me that PackageReference is what Microsoft will support moving forward. I wouldn't be surprised if VS2018/VS2019 made PackageReference the default option when creating a new project - and that would cause friction for our users.

I think 1 and 2 are important for making it easy to get started and making it safe to uninstall the package and re-install. In the past this was more important, since uninstall and reinstall was the easiest way to fix the debugger settings etc.

NuGet by default would only remove the .dna file from the project, if the file is identical to the one that came with the package (i.e. user didn't change anything), and when reinstalling, NuGet doesn't replace the file unless the user tells it to, thus we can make it safe to uninstall and reinstall, without the current features in install.ps1.

The main difference is that that the .dna file no longer would have the name of the project - it would be a static file name that we define.

---

We document the manual steps that should be taken to make PackageReference work:
add a .dna file

We can have a .dna file added to the project when supporting PackageReference.

set the reference to ExcelDna.Integration.dll to be Copy Local=false

Perhaps something for a separate discussion, but we could get rid of this step too if we wanted, and not have ExcelDna.Integration.dll embedded in the .xll by default, and load whatever comes with the package (and make sure it's packed when ExcelDnaPack runs).

set up the debugger paths.

This is no longer on the Powershell, so if we update the current nuget package to add the .targets file using the NuGet feature, the build pipeline would work with PackageReference as well

[govert]

@screig Note that the ExcelDna.AddIn v0.34 NuGet package already deals with the packing in the custom build task, and no longer adds or requires any post-build steps. It might be nice to incorporate the packing code into the add-in tasks assembly, but that would make no difference to the current issue.

@augustoproiete I am not willing to complicate or change the current-style NuGet experience for the sake of the so-far rarely used and problematic PackageReference style. We've just formalised how to deal with .dna file targeting 32-bit and 64-bit etc. in the great work you did for v 0.34. I would like this to be stable for a few years - and perhaps even document it a bit better than here: https://github.com/Excel-DNA/ExcelDna/wiki/Build-Output-Customization.

Future VS versions might change how we think about PackageReference, but it is many years premature to worry much about that now. In its current state it's a mess (not just for Excel-DNA) and seems to me irrelevant to normal desktop development.

[screig]

@govert I stayed on 33.9 because I had constructed a custom post build event and I have seen an upgrade wipe it out. I will try v34, Thanks!

Edit, just FYI, switching over to 34.6 means I don't need a post build event, however the ribbon no longer loads.

[screig]

well I would up-vote for the new style, not because of the new package reference style per say. All my other projects that I have my ExcelDna project referencing, are now net standard. Calling Net Standard from Net Framework should be seamless but it is not. It would be cleaner if this were net-standard too.

[augustoproiete]

Re PackageReference not having support for install.ps1, it seems Microsoft might be working on something to resolve that: NuGet/Home#5963

[govert]

@augustoproiete That NuGet issue is from September last year. What signs of movement do you see?

[govert]

@augustoproiete For us an easier start might be to try to move ExcelDna.Integration so it works with PackageReference. It just uses the install.ps1 to set Copy Local = false.

[augustoproiete]

@augustoproiete That NuGet issue is from September last year. What signs of movement do you see?

/cc @govert

[augustoproiete]

@augustoproiete For us an easier start might be to try to move ExcelDna.Integration so it works with PackageReference. It just uses the install.ps1 to set Copy Local = false.

Good idea!

[govert]

@augustoproiete OK, they've set some tags. There is no suggestion on the table yet of what to do. The ExcelDna.Integration case is funny in this context - we really need something called a "Reference assembly" that doesn't have any code. Internally they have tools that make these, but it has taken several years of promises, issues and "tag" settings and I think it has still not shipped in Roslyn.

I'm happy to consider a configuration for our packages that works with PackageReference, but I'm not sure why we're letting the NuGet guys stuff us around. But I suspect if we wait a year or two they'll figure out that the need significantly more flexibility in what the package can do if they want to drop the general install.ps1 PowerShell support.

[jacobneroth]

Is there a current workaround for PackageReference issue ? meaning is it possible to run the install.ps1 manually

[govert]

I think you one can manually set up the .dna file, which is the main work the install.ps1 does.
I'd recommend starting with the latest pre-release from NuGet (which does the debug settings in the build task) and figuring out what to do from there.

It would be useful as a start to make some instructions for setting things up manually, if you have some time to figure it out.

[govert]

FWIW, there was a recentish response to this issue: NuGet/Home#6330

[jacobneroth]

I think you one can manually set up the .dna file, which is the main work the install.ps1 does.
I'd recommend starting with the latest pre-release from NuGet (which does the debug settings in the build task) and figuring out what to do from there.

It would be useful as a start to make some instructions for setting things up manually, if you have some time to figure it out.

Sadly that didn't work but i will try out creating a brand new project and see how it works and perform the operation together.

[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]

Closing this in favour of #188 for tracking the PackageReference support.