ExcelDna icon indicating copy to clipboard operation
ExcelDna copied to clipboard

Use Dot Net Core SDK direct from the install tree rather than a copy.

Open davidhunter22 opened this issue 2 years ago • 13 comments

There seems to be a copy of the 32 bit libnethost.lib and header files inside ExcelDna.Host. An alternative would be to reference them from the SDK install tree. I don't think it's unreasonable to say that installing the SDk is a prerequisite of building ExcelDNA itself. I will create a PR so you can take a look, this will include getting the 64 bit build of ExcelDna.Host to work as well.

You mentioned dealing with the packed ExcelDna.ManagedHost.dll file. When you say "dealing with" do you mean loading into the CLR in the ExcelDna.Host code?

I do have otehr build suggestions, the following are roughly in order of which I think might be most impactful.

  1. Generate the nuget packages as part of the csproj files rather than using nuspec files and batch commands, this uses <GeneratePackageOnBuild>. This way the packages generation is simply part of the normal build process with no other messing around. This could also include symbols via <IncludeSymbols>. Note this is orthogonal to the issues of users consuming the nuget package via <PackageReference> rather than packages.config.
  2. Factor out more build stuff into Directory.Build.props
  3. Put all build output into a common solution wide output directory rather than into each project and then copying stuff around
  4. Select a C# compiler version, via <LangVersion>, I would suggest 9.0
  5. Switch on nullabel reference types.
  6. use /W4 and treat warnings as erros.
  7. Use static analysis tools

davidhunter22 avatar Jul 19 '21 14:07 davidhunter22

I think it's simpler to keep a separate copy of the nethost files. There is a way to reference a C++ package to get them, but that seems more complicated. I'm trying to keep stuff less complicated for now. Referencing from the SDK has all kinds of version issues.

I suggest we not fiddle too much with the build stuff for now, please. I definitely can't handle large-scale changes to the project (at least for now), so I think you'll be wasting your time there - sorry. And I don't want to refactor the managed code right now, unless we need to make functional changes. The managed code mostly needs to run under .NET 4.5.2 too.

There is a good opportunity to help with the .NET 5+ hosting, since that's not baked at all yet. I've created a good issue to start with here: https://github.com/Excel-DNA/ExcelDna/issues/398

govert avatar Jul 19 '21 14:07 govert

Another issue that can do with help without affecting the existing stuff is the whole PackageReference story: https://github.com/Excel-DNA/ExcelDna/issues/363

I don't know exactly how that should work yet, but you might have some suggestions based on my first dump of ideas in that issue.

And I think your suggestion 1. above is a good one, but it seemed to me that building the packages as part of the csproj files would be hard. So maybe a separate project to build the packages would be feasible.

govert avatar Jul 19 '21 15:07 govert

OK I'll look at the <PackageReference> issues, I assume you mean users of being able to use <PackageReference> rather than package.config.

BTW I would suggest that you target .NET Core 6 which is in preview rather than 5. Version 5 will be end of lifed 3 months after 6 comes out in November. 6 is an LTS version so will supported for 3 years.

davidhunter22 avatar Jul 19 '21 15:07 davidhunter22

The current packages have a Powershell install.ps1 script that manipulates the project file, but the PackageReference style references (which are required in SDK-style project files) do not run the script. So we need a different mechanism make it so that an SDK-style project can easily use the Excel-DNA package to create an add-in. Most of the issue has to do with the .dna file which directs the add-in, and setting up the debugging to work.

I think you're right that we might target .NET 6 for a release of the .NET 5+ support (since .NET 5+ doesn't support side-by-side runtimes), but for now we might as well work with the current version. I don't know of any relevant differences at the moment.

govert avatar Jul 19 '21 15:07 govert

Yes I had already looked at the script.

Given the script and the manipulation of the csproj file which is obviously very different in and SDK style project are you thinking of two nuget packages, one for SDK and for for legacy style csproj files or do you want to try to do one that works for both?

davidhunter22 avatar Jul 19 '21 16:07 davidhunter22

I think for a user to change from an old-style project file to an SDK-style project is quite disruptive anyway. So if there are incompatible changes or a different package name, then it would not be a big problem. So for me the main downside of a different package is that it becomes harder to document, discover and pick the right one.

So I'd say for figuring out how to do it a separate package is easier, and then we can see whether there is a way to incorporate support for SDK-style projects in the existing package.

govert avatar Jul 20 '21 04:07 govert

I think you're right that we might target .NET 6 for a release of the .NET 5+ support (since .NET 5+ doesn't support side-by-side runtimes), but for now we might as well work with the current version.

@govert, just from a technical standpoint, I'm curious: does .NET 6 support loading multiple versions of the CLR into the same process? If not, then what's the use case for supporting .NET 6 in Excel-DNA?

Just curious because we're investigating the options for upgrading our VSTO addin from .NET Framework to .NET 5 (or 6). But it seems like we're blocked due to these issues:

  • https://github.com/dotnet/runtime/issues/12018 (which you also commented on some years ago)
  • https://github.com/dotnet/core/issues/5156

I.e. it's not feasible for us to target .NET Core/5/6 if it doesn't support loading multiple versions of the runtime into the same process, since we will potentially break other addins targeting .NET Core/5/6 (though, it seems like loading .NET Core/5/6 and .NET Framework side-by-side is fine besides some relatively minor issues that you also comment on in the first issue that I linked to).

So hence my question: what's the use case for Excel-DNA in supporting .NET 6? Have things changed in that version of .NET with regards to side-by-side loading? (I haven't been able to find any documentation confirming that). Isn't it "dangerous" to go down that route?

sbolofsson avatar Jul 24 '21 19:07 sbolofsson

@sbolofsson My understanding and experience so far is that .NET Framework 4.x can be loaded at the same time as .NET 5, and that this is in fact a supported scenario. I would presume that nothing in this regard will change with .NET 6, so that .NET Framework 4.x and .NET 6 can also be loaded in the same process. However, .NET 5 and .NET 6 won't be able to load in the same process, as you point out.

Since Excel-DNA does not support .NET 5 or .NET 6, and possible support for these is still a while away, it will probably make sense to only support .NET 6 (which is also a long-term support release). That way the side-by-side issue between .NET 5 and .NET 6 never appears, and we can reevaluate in later years how to deal with future versions .NET 7+. But being 'stuck' on .NET 6 might be a viable option for a few years, and I can imagine workarounds after that if need be.

govert avatar Jul 24 '21 21:07 govert

@govert, totally follow you in the scenario with .NET 6 and .NET Framework 4.x being able to be loaded into the same process simultaneously - that's good. And also on the part about being 'stuck' on .NET 6, which is an LTS release thereby making it okay to just 'skip/disregard' .NET 5, since it will sooner be unsupported anyway.

But there could be other addins (both Excel-DNA and non Excel-DNA) targeting different minor versions of .NET 6 loaded into the same process, thereby causing issues. What are your thoughts on this?

I don't know if future minor versions of .NET 6 will imply a different version of the runtime. I haven't been following .NET Core/5/6 closely enough to be sure of how they version the runtime vs. the framework. (In the old .NET Framework it was easier to follow due to the almost never changing runtime version - but I guess that's exactly not a design goal of the new .NET).

sbolofsson avatar Jul 25 '21 06:07 sbolofsson

Yes, I guess there might eventually be problems dealing with different .NET 5+ versions.

I can't tell yet whether we'll be able to support any such version with Excel-DNA, for both technical and effort reasons, so the theoretical version issues seem like a bit of a luxury to consider now.

govert avatar Jul 25 '21 14:07 govert

Got it. I thought this issue was about supporting .NET Core/5/6 in which case I think the version issues are not theoretical but a necessity to discuss :)

None the less, I was also trying to draw parallels from Excel-DNA to our own situation. And yes we also use Excel-DNA - Many thanks for an ingenious piece of software 🙂

sbolofsson avatar Jul 25 '21 17:07 sbolofsson

Are contributions from the community welcome regarding .NET Core support? Is there anything in particular where we could help @govert and @augustoproiete?

gmichaud avatar Sep 15 '21 19:09 gmichaud

Hi @gmichaud - yes certainly any help would be welcome, though you have to take it a bit slow with me.

Your first step would be to figure out the current state of the .NET 5+ support. The current source tree has both the native host project (ExcelDna.Host) and a test add-in (ExcelDna.Test). On my machine I can set the ExcelDna.Test project as the startup and it works. You might need to tune the launchsettings.json, or the pre / post build steps to get the right files set up.

So for me this add-in loads and UDF functions, RTD and the ribbon all work.

  • Can you make it work?
  • Do you see any problems?

Once you have the basics working, there are some specific themes that need attention:

Native host bootstrapping and error handling.

Currently the managed bootstrapper, .dna file and runtimeconfig files are not packed into the .xll. So you have to have these as separate file for the add-in. See https://github.com/Excel-DNA/ExcelDna/issues/398 image

SDK-style project file / PackageReference support

This is best addressed in issue https://github.com/Excel-DNA/ExcelDna/issues/363 As a first step, some discussion and thoughts about the design, compatibility etc. for how this should work would be very welcome.


In general it helps me if you can open or extend an issue with some discussion on the intended scope of changes, before making a pull request. You are also welcome to get in touch with me directly and set up a chat.

govert avatar Sep 15 '21 19:09 govert