-
-
Notifications
You must be signed in to change notification settings - Fork 924
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
Feature/assembly strongname #3265
Feature/assembly strongname #3265
Conversation
Bumps [Moq](https://github.com/moq/moq4) from 4.18.1 to 4.18.2. - [Release notes](https://github.com/moq/moq4/releases) - [Changelog](https://github.com/moq/moq4/blob/main/CHANGELOG.md) - [Commits](devlooped/moq@v4.18.1...v4.18.2) --- updated-dependencies: - dependency-name: Moq dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…timal cross-runtime compatibility (or really just for netframework)
Desperately need this! |
I looked through much of the available MS documentation on this topic. I cannot find any significant reason not to sign the assembly. Here's what I noticed:
I support these changes. However, I suggest that we target the @anderslaub Can you modify your PR to target the |
@anderslaub Also, can you add a PR targeting the |
@Shane32 I've changed PR to target Now I noticed the dependencies from the However, I can't help notice the cyclic dependency flow. I can't build the full It's not a blocker at all; The needed references from My point is really just that to me this indicates that PR coming up in |
You make a very good point; I've been thinking that we should stop publishing samples that do not use the server project, as many people copy from these samples which contain non-recommended design patterns. However, I don't think this will be updated prior to 7.0 release. cc: @sungam3r |
@Shane32 Pragmatically; it works, so.. Another question, I've made signed nugets from It wouldn't be a problem for me to make some new temporary nugets that has strong named assemblies from another branch - but I can't find any branches or tags for 6.0.0-preview-612 - is it just Or should I just leave it and PR into |
It's the |
|
and the reference in |
If all is well and @sungam3r has no comments, I'll merge and release the parser 8.1.0, then you can fix up the GraphQL.NET PR, and we will merge that, and hopefully release 7.0. Once 7.0 is released, you can update the PR for the GraphQL.NET Server PR, and I'll merge and release 7.0 of GraphQL.NET Server. We are just waiting for PR #3245 to be finished before we can release 7.0. |
We renamed 6.x to 7.x to align with server repo. There is no 6.x. Probably it's just an older beta build. Here's all the latest beta builds of GraphQL.NET. You'll notice 5.x or 7.x builds based on when a PR got merged to either master or develop last. So the latest 7.x preview is 7.0.0-preview-634 and really the server repo should be referencing that rather than 6.0.0-preview-612. https://github.com/graphql-dotnet/graphql-dotnet/packages/433360/versions |
When we release GraphQL.NET 7.0.0 we will update the reference in the server repo to 7.0.0 of course. |
Once the PR in the parser library is merged, even before 8.1.0 is released, we can reference the beta parser lib in GraphQL.NET so it is referencing a strongly-named assembly. Ditto for GraphQL.NET/Server. |
Sure okay, then I just wonder why the test projects on Btw; do you think there is any chance of getting a 2.4.1 release out with strong names. 2.4.0 seems to be where a lot of net framework solutions are stuck. I can do the signing etc. in a hotfix branch off 2.4.0 + in parser if it would be possible to get it released. |
I fail to understand why we would do that, or what basis there is for saying that .NET Framework solutions are either stuck or using 2.4.0. The current version of GraphQL.NET only requires .NET Standard 2.0, which is supported by .NET Framework 4.6.1 and later. Support for .NET Framework 4.6.1 and prior versions ended in April. .NET Framework 4.6.2, still supported, is compatible with .NET Standard 2.0 and hence compatible with GraphQL.NET. So while lack of support for .NET Framework 4.5 may have been a blocker in the past, it seems a bit irrelevant now. We can also view the NuGet statistics for GraphQL.NET which show that in the last 6 weeks, while there were 134,000 downloads of v2.4.0 (which was the current version for a 2-year period), there was 450,000 downloads of newer versions (also covering about a 2-year period). So while there are a significant number of people still on 2.4.0, the number seems about right considering the length of time it was the active version. Also, 2.4.0 was missing some major features; 3.0.0 was a huge step forward in many ways, as was 4.0 for speed and 4.6 for ease of configuration and 5.0 for type-driven graphs. With the upcoming 7.x version, it will be easier than ever before to spin up a new GraphQL server within .NET -- and for .NET Framework users especially, with the server's project support of ASP.NET Core 2.1 running on .NET Framework. As it pertains to a hotfix, I have no problem issuing hotfixes for older versions of GraphQL.NET. However, the deployment scripts currently in use were written for 3.1.0 and later, so it is not easy/practical to release a hotfix for a version prior to 3.x. To put it simply, we can't issue a 2.4.1. I also fail to see the logic here; anyone using 2.4.0 now is likely using it because they have a current deployment utilizing that version. A strong name is not going to help them. Whereas a newcomer to GraphQL.NET that may require a strong-named assembly is not going to select 2.4.1 over the current version. |
@Shane32 Let me try to explain; It's not about the current version or lack of .NET Framework support at all. It is inherited dependencies combined with the legacy of .NET Framework assembly binding that is the PITA. The problem is as you know, that without a signed "strong" name on the assembly you cannot do any version assembly binding. .NET Framework ignores the version of an assembly if it is not signed. This is really good if there are no breaking changes between versions - but if there is and you have external dependencies that depend on a specific version then you are locked to that version or you break your dependencies at runtime. To use my own situation as an example; I am upgrading a custom codebase that is built for a closed-source .NET Framework based CMS. The custom codebase that I am upgrading use GraphQl v4.5. The version of the CMS that it was built for did not depend on GraphQl - however, the new version of the CMS has new modules that depend on GraphQl v2.4. So without a strongname on neither v2.4 or v4.5 there would be nothing to do but downgrade the codebase to target v2.4.0 - which would be a lot of work and really just plain stupid - or alternately, try to convince the CMS vendor to upgrade their dependency but they might also be locked in on v2.4.0 due to one of their dependencies - or at least due to internal dependencies that all would have to be upgraded at the same time too. To fix this I've simply packed up a custom signed v4.5.0.0 - which is what brought me here in the first place. With a strongname on the v4.5.0 assembly I can do a runtime version based assembly binding like this: <dependentAssembly xdt:Transform="InsertIfMissing" xdt:Locator="Condition(asmv1:assemblyIdentity/@name='GraphQl')">
<assemblyIdentity name="GraphQl" publicKeyToken="e6238258560628ee" />
<codeBase version="4.5.0.0" href="bin\deps\GraphQL.dll" />
</dependentAssembly> And this just works - the inherited old unsigned v2.4.0 dll remains in the bin/ folder and when the code requests a v4.5.0 it use the assembly from the specified location. This solved my specific issue in this particular situation but it doesn't help me next time and it doesn't help others who are in a similar situation. They would also have to "roll their own" signed version and fiddle around with the build output before deploying their code. For the CMS vendor, they have a significant large task in upgrading their dependency on v2.4, especially since they have several modules which all have to be upgraded at once since the assemblies are unsigned. Without a strong name, they cannot do it one module at a time using codebase version bindings. With a signed v2.4.1 this would be much easier for them and it would also be somewhat easier for us "consumers" to assembly bind the other way. Move the old version out of the bin/ folder for legacy dependencies, rather than moving the new version out. Makes sense? I don't know if my thesis about many .NET Frameworks being stuck on v2.4.0 is correct. But I do know a multitude of solutions that are build on the same CMS which are currently stuck. They cannot use a newer version of GraphQl before a signed version is released. I doubt that this particular vendor is the only .NET Framework based platform who has been using GraphQl since v2.4.0 and haven't upgraded their codebase to a newer version yet - thus are making all solutions built on top of their platforms stuck on that version. |
As it relates to your previous comment, I think I follow. It is possible to release a 2.4.1 but we would need a PR to bring in the proper GitHub Action workflows from 3.1.0 or newer, and possibly update them to work with the older .NET Framework referenced by the project. That may be a bit pointless unless we first release GraphQL-Parser 3.0.1 with a strong name. |
@anderslaub If you bump GraphQL-Parser to |
Codecov Report
@@ Coverage Diff @@
## develop #3265 +/- ##
===========================================
- Coverage 84.21% 84.20% -0.02%
===========================================
Files 374 374
Lines 16075 16075
Branches 2588 2588
===========================================
- Hits 13538 13536 -2
- Misses 1917 1918 +1
- Partials 620 621 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Well, all the .NET Framework 4.8 tests failed (which only run on Windows), with an error about the strong name not matching.
.NET Core doesn't check strong names, so all those tests passed. |
@anderslaub I merged in #3278 and then merged develop into your branch. Then I fixed the workflows in your branch here so that it would pass the tests. Apparently when calculating the code coverage, something breaks the strong name. But since we don't use the code coverage reports on Windows, I disabled the build process from calculating the code coverage on Windows -- code coverage is still calculated on Linux and uploaded as it has always done. |
@sungam3r Please review if you like |
@sungam3r I'm merging this so we can get the strong name PR done in the server repo. Lmk if you have any comments. |
And... it has nothing to do with signing problem. Regarding samples with or withous server project - I'm 50/50 here and have no strong opinion. |
This is coverlet magic, it instruments assembly to insert special instructions to track code coverage. |
@@ -1 +0,0 @@ | |||
[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("GraphQL.MicrosoftDI.Tests")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I did not notice these two deleted files.
Motivation
Many solutions based on .NET Framework (net4x) has both direct and inherited dependencies on GraphQl-dotnet.
Since the currently released assemblies does not have a strongly typed name, developers who work on such solutions are forced to depend on a compatible version of graphql-dotnet as their inherited dependency. This often means v2.4.0 due to breaking changes introduced in later major versions.
By strong name signing the assemblies that target netstandard2.0/1, developers who are currently stuck on an old inherited version will be able to do codebase versioned assembly binding.
example:
Signing the assemblies also enable other use-cases which has otherwise been prevented by legacy requirements for strong-named assembly (such as Visual Studio extension development).
The lack of a strong name on current releases prevents developers from transitioning their codebases to later versions of GraphQl leaving many projects stuck on graphql-dotnet v2.4.0
As support for this thesis, the nuget.org download stats clearly shows that v2.4.0 is by far the most downloaded nuget and continues to have a very significant amount of daily downloads.
Notes
Pushing the private key used for signing the assemblies is the recommended approach for open-source projects and is not a security concern.
We can all agree that signing assemblies for managing dependencies should never have been a thing or should at least be a thing of the past. This is unfortunately not the reality yet and leaving this out on popular frameworks such as this makes modernizing code-bases even harder.
Considerations
Assembly sigining could alternatively be done in a Github action.
The reason for having it as part of the build is that it provide more flexibility and also enable local builds to be signed with the same key as the officially released assemblies - making testing and extending less prone to error.
Important
Since strong named assemblies only can reference other strongly named assemblies. These 2 PRs are dependent:
References
Microsoft recommend publicly published .NET libraries and open-source maintainers to sign their assemblies for best cross-runtime compatibility.