plotly / Plotly.NET

interactive graphing library for .NET programming languages :chart_with_upwards_trend:
https://plotly.net
MIT License
662 stars 85 forks source link

Strongly named assembly #175

Closed WhiteBlackGoose closed 2 years ago

WhiteBlackGoose commented 3 years ago

Should sign all assemblies in the project (mb except the *.Interactive?) to make sure they can be referenced by strogly named assemblies

kMutagene commented 3 years ago

Can you give some info on what this means / why it is needed? This is absolutely out of my domain^^

WhiteBlackGoose commented 3 years ago

It's a thing which allows to make sure that the used assembly is the right one (e. g. it comes in when there are assemblies with the same name). Not very much of a use, and it is only useful for .NET Framework. But those users who do rely on this thing, they have to have all their deps strongly named too. So if your assembly is not strongly named, they won't be able to use the lib at all.

I'm not saying it's a big issue though, and definitely not urgent. But a day may come when someone won't be able to use the lib because of it.

JakeRadMSFT commented 2 years ago

Hello!

I'm Jake from the ML.NET team (one of .NET's Machine Learning libraries) - we're creating a bunch of sample notebooks and we'd love to use Plotly.NET in some of our Interactive Extensions. Unfortunately, we can't take dependency on Plotly.NET until it's signed.

Notebooks are an important part of our strategy going forward -

We have them in Visual Studio https://marketplace.visualstudio.com/items?itemName=MLNET.notebook

and we're working on adding a collection of getting started with ML notebooks here: https://github.com/dotnet/csharp-notebooks

We'd also like to use Plotly.NET in Model Builder (That is what LittleLittleCloud was asking about above). https://dotnet.microsoft.com/en-us/apps/machinelearning-ai/ml-dotnet/model-builder

How can we help move this forward? I'm hoping this would help both of our products grow :).

kMutagene commented 2 years ago

Hi @JakeRadMSFT , thanks for reaching out! Integration with ML.NET samples is definitely something that increases the priority of this issue quite a bit!

What i don't want to happen is implementing this while not understanding it and then have issues down the line due to it. I have read a bit into strong named assemblies and i am a little bit confused about the concept, so here are some questions/things i need to understand before implementing this.:

Also, i have read in some threads that this basically breaks all dependent projects once, not sure if that's true. If so, this would require a major version increase i guess?

All together, the essence of threads such as this one or this one seems to be that this is legacy stuff but it cannot hurt to support it.

The thing is that i cannot control that for third party libraries. This seems like the exact thing that causes development workflow issues down the line. How should we handle third-party dependencies?

kMutagene commented 2 years ago

On another note, from your link it looks like you will write the notebooks in C#. Depending on the complexity of visualizations you want to show, you might run into the issues outlined in #285. Input (or even help) on that issue from the C# user perspective would be awesome.

JakeRadMSFT commented 2 years ago

Thanks @kMutagene - I'll be honest - I've never signed an assembly outside of Microsoft. This would be a good learning experience for me on best practices.

Also, i have read in some threads that this basically breaks all dependent projects once, not sure if that's true. If so, this would require a major version increase i guess?

This is sort of implied from above but I've also never tried to transition a NuGet to signed. Perhaps there are issues there - - that might be why libraries like Markdig have a signed and unsigned version.

https://www.nuget.org/packages/Markdig/ https://www.nuget.org/packages/Markdig.Signed/

Newtonsoft.Json, which seems to cause all kinds of problems with strong naming i have to admit i do not understand, but that might be outdated.

Newtonsoft.Json certainly causes some issues. It's bitten me more than a few times :). It essentially comes down to Dependency A wants Newtonsoft 9.0 Dependency B wants Newtonsoft 10.0 ... something needs to resolve the discrepancy. These are generally resolved with binding redirects and it's a pain when it gets all the way to runtime and you hit these issue. Usually this happens in dynamically loading assemblies or other extensibility situations. I'm sure I could find someone with a better explanation if you'd like.

@kMutagene What are your thoughts on having two NuGets? Should I try to wire that up? That way we don't break existing users.

kMutagene commented 2 years ago

before we get deeper into this, what is the exact reason why the assembly must be signed? Are you making non-.NET(core) notebooks? If i understood this right, strong names are only relevant for netfx:

For .NET Core and .NET 5+, strong-named assemblies do not provide material benefits. The runtime never validates the strong-name signature, nor does it use the strong-name for assembly binding.

https://docs.microsoft.com/en-us/dotnet/standard/assembly/strong-named#why-strong-name-your-assemblies

kMutagene commented 2 years ago

also the official guidelines discourage publishing two versions of the package: https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/strong-naming (although this looks like the easiest solution for me).

Anyways, we still have the problem of third-party libraries we are using not being signed.

JakeRadMSFT commented 2 years ago

For .NET Core and .NET 5+, strong-named assemblies do not provide material benefits. The runtime never validates the strong-name signature, nor does it use the strong-name for assembly binding.

Let me dig in on this with the experts internally.

Ultimately, there is a policy in the https://github.com/dotnet org's repos and builds that enforces signing. We also want to use this inside of Visual Studio's Model Builder and that is .NET Framework ... which requires strong-named.

also the official guidelines discourage publishing two versions of the package: https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/strong-naming (although this looks like the easiest solution for me).

I'll learn more about this too.

JakeRadMSFT commented 2 years ago

Anyways, we still have the problem of third-party libraries we are using not being signed.

It looks like you guys don't have too many!

JakeRadMSFT commented 2 years ago

Okay - I've reached out to some experts and re-read the documentation.

My Thoughts:

Let me know your thoughts! I'm happy to help wire this up for Plotly.NET and DynamicObj. I believe this is a one time cost (and maybe some issue handling) and shouldn't have any more work associated with it.

FYI - I don't have strong opinions on versioning or shipping multiple NuGets. If you like your versioning ( and don't want to bump to 3 just for signing) ... the two NuGet way works for me too.

kMutagene commented 2 years ago

Thanks for the insights!

I think my preferred approach would be:

If you could help wiring that up, that would be great. I think we could start with DynamicObj, basically using it for a first dry-run of this process and then apply the same procedure to Plotly.NET.

kMutagene commented 2 years ago

@JakeRadMSFT I think we are already quite close for wrapping this up for DynamicObj. My last question concerns the AssemblyVersion. Per https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/versioning:

CONSIDER only including a major version in the AssemblyVersion. e.g. Library 1.0 and Library 1.0.1 both have an AssemblyVersion of 1.0.0.0, while Library 2.0 has AssemblyVersion of 2.0.0.0. When the assembly version changes less often, it reduces binding redirects.

How do we achieve this decoupling of nuget and assembly version? The current build pipeline increases both the assembly and package version (i have tested this by locally bumping the version to 2.0.1):

image

You can also see that the dependencies are all doing this version decoupling (FSharp.Core, netstandard, Newtonsoft.Json)

So i think the last roadblock here is:

JakeRadMSFT commented 2 years ago

@kMutagene let me ask some folks! Sorry I missed these messages!!

JakeRadMSFT commented 2 years ago

My guess is that you're build process is somehow setting the version property via msbuild somewhere which then overrides all the versions.

Packages like newtonsoft set each version: https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Newtonsoft.Json.csproj#L7

I believe we can do this from msbuild by setting each version property "AssemblyVersion", "FileVersion" ... instead of setting "version".

https://github.com/JamesNK/Newtonsoft.Json/blob/bf2e2a78e8febf0006ec647f9bde3aa5bbe0ce72/Build/build.ps1#L164

exec { & $script:msBuildPath "/t:build" "/v:$msbuildVerbosity" $projectPath "/p:Configuration=Release" "/p:LibraryFrameworks="$libraryFrameworks"" "/p:TestFrameworks="$testFrameworks"" "/p:AssemblyOriginatorKeyFile=$signKeyPath" "/p:SignAssembly=$signAssemblies" "/p:TreatWarningsAsErrors=$treatWarningsAsErrors" "/p:AdditionalConstants=$additionalConstants" "/p:GeneratePackageOnBuild=$buildNuGet" "/p:ContinuousIntegrationBuild=true" "/p:PackageId=$packageId" "/p:VersionPrefix=$majorWithReleaseVersion" "/p:VersionSuffix=$nugetPrerelease" "/p:AssemblyVersion=$assemblyVersion" "/p:FileVersion=$version" "/m" }

JakeRadMSFT commented 2 years ago

I've never used the build tools you're using (FAKE) but I think it's this:

https://github.com/plotly/Plotly.NET/blob/5c6f3591aa7bd06b1cab3977a16b1f16aba2c26a/build/PackageTasks.fs#L21-L26

It looks like it's setting all versions based on NuGet symver ... https://github.com/plotly/Plotly.NET/blob/5c6f3591aa7bd06b1cab3977a16b1f16aba2c26a/build/ProjectInfo.fs#L31-L35

kMutagene commented 2 years ago

@JakeRadMSFT exactly, both versions can be set via msbuild params, that was the missing link. The signed version of DynamicObj is live. We can do the same for Plotly.NET soon, i am just wondering if it would be better to get #294 merged first and release the Plotly.NET.CSharp package alongside the 3.0 packages. So this kind of goes in the direction of my question in #285: What's the minimum functionality you would need for getting the notebooks running? Specific charts only? Is default styling enough?

JakeRadMSFT commented 2 years ago

I'll reply in #285 :)