Skip to content
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

Sign all packages and remove .Signed packages #539

Merged
merged 6 commits into from
Nov 6, 2018

Conversation

tmilnthorp
Copy link
Collaborator

@tmilnthorp tmilnthorp commented Oct 29, 2018

  • Remove .Signed projects and nuget packages
  • Sign all libs except UnitsNet.WindowsRuntimeComponent, it was not previously signed
  • Set AssemblyVersion to 4.0.0.0 for UnitsNet and JSON lib
  • Updated version-scripts to set AssemblyVersion as major part only of Version
  • Changed title to suffix - DEPRECATED
  • Changed description to prefix DEPRECATED! Use UnitsNet 4.0.0 or later instead, which is now signed. This package will be unlisted sometime later. [...old description...]

@tmilnthorp
Copy link
Collaborator Author

tmilnthorp commented Oct 29, 2018

Mark as deprecated in master and push a new release, then delete the .signed assemblies in v4? This is going to have the same issue with the json tests BTW. Need to resolve.

@angularsen
Copy link
Owner

On home guard duty and only phone out this week so limited feedback here, but I wonder what is the best practice for deprecating packages.
Description alone is not sufficient I think.

Adding DEPRECATED to title or unlisting package from search might be better?

Also, as per guideline we need to ensure assembly version uses only major version (3.0.0.0 vs 3.190.0) to avoid problems with assembly binding redirects for consumers.

@angularsen
Copy link
Owner

angularsen commented Nov 2, 2018

Apparently there is no support for marking packages as deprecated, but there is some work happening on this:
https://github.com/NuGet/Home/wiki/Deprecate-packages
NuGet/Home#2867

I suggest the following

  • Change title to Units.NET (signed) - DEPRECATED
  • Change description to DEPRECATED! Use UnitsNet 4.0.0 or later instead, which is now signed. This package will be unlisted sometime later. [...old description...]
  • Ensure versioning scripts set only major versions of assembly version (ex: 4.0.0.0 not 4.0.4.0) to only require an update in assembly binding redirects when upgrading to a new major version
  • Set existing assembly version to 3/4.0.0.0 if not already set

@angularsen angularsen changed the base branch from master to v4 November 4, 2018 11:35
@angularsen angularsen added this to the 4.0 milestone Nov 4, 2018
@angularsen
Copy link
Owner

It seems AssemblyVersion and FileVersion is already set correctly.
image

@angularsen
Copy link
Owner

Disregard that, I just tried with 4.0.1-alpha5 and that shows versions 4.0.1.0 in dotpeek. So needs fixing.
I'm looking at this right now.

@angularsen
Copy link
Owner

Now fixed. I chose to let FileVersion remain major-version (same as AssemblyVersion), because that is the default behavior and it has no practical effect besides what is shown when right-clicking in Windows - Properties - Details. Only the nuget package holdes the full version, and its binaries have assembly informational version set by SourceLink:

If you're using SourceLink, this version will be set on build with the NuGet package version plus a source control version. For example, 1.0.0-beta1+204ff0a includes the commit hash of the source code the assembly was built from. For more information, see SourceLink.

Locally built binary:
image

@angularsen
Copy link
Owner

I just discovered that signing UnitsNet nuget is indeed a breaking change for JSON serialization.

The compatibility tests using 1.3.0 JSON nuget fail with the following:

      System.MethodAccessException : Attempt by method 'UnitsNet.Serialization.JsonNet.UnitsNetJsonConverter.GetUnitFullNameOfQuantity(System.Object, System.Type)' to access method 'UnitsNet.InternalHelpers.ReflectionBridgeExtensions.GetPropety(System.Type, System.String)' failed.
      Stack Trace:
           at UnitsNet.Serialization.JsonNet.UnitsNetJsonConverter.GetUnitFullNameOfQuantity(Object obj, Type quantityType)
           at UnitsNet.Serialization.JsonNet.UnitsNetJsonConverter.WriteJson(JsonWriter writer, Object obj, JsonSerializer serializer)
           at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeConvertable(JsonWriter writer, JsonConverter converter, Object value, JsonContract contract, JsonContainerContract collectionContract, JsonProperty containerProperty)
           at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty)
           at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.Serialize(JsonWriter jsonWriter, Object value, Type objectType)
           at Newtonsoft.Json.JsonSerializer.SerializeInternal(JsonWriter jsonWriter, Object value, Type objectType)
           at Newtonsoft.Json.JsonConvert.SerializeObjectInternal(Object value, Type type, JsonSerializer jsonSerializer)
        C:\dev\unitsnet\UnitsNet.Serialization.JsonNet.Tests\UnitsNetJsonConverterTests.cs(40,0): at UnitsNet.Serialization.JsonNet.Tests.UnitsNetJsonConverterTests.SerializeObject(Object obj)
        C:\dev\unitsnet\UnitsNet.Serialization.JsonNet.Tests\UnitsNetJsonConverterTests.cs(177,0): at UnitsNet.Serialization.JsonNet.Tests.UnitsNetJsonConverterTests.Deserialize.NonNullNullableValueNestedInObject_ExpectValueDeserializedCorrectly()

This is because [InternalsVisibleTo] in a signed assembly can only expose its internals to other signed assemblies, and the 1.3.0 JSON nuget is not signed (or, we have a separate signed package).

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs1726

A strong name signed assembly can only grant friend assembly access, made with the InternalsVisibleToAttribute, to other strongly signed assemblies.

TL;DR I believe we need to

  • Alternative 1: Restrict JSON 1.x nuget to UnitsNet 3.x - BUT, this is currently not possible without going back to .nuspec files, which I really don't want to
  • Alternative 2: Bump major version of JSON nuget to 4.x to match UnitsNet (this is already done on v4 branch), to indicate a breaking change moving to signed packages. Consumers will simply have to update both libs to 4.x to continue business as usual.

@angularsen angularsen changed the title Signing packages in favor of separate .Signed assemblies. Sign all packages and remove .Signed packages Nov 4, 2018
@angularsen
Copy link
Owner

Re-verified that artifacts from this PR are now signed and has the proper versions.

image

tmilnthorp and others added 5 commits November 4, 2018 21:03
Set to major version (x.0.0.0) for version x.y.z as per recommendation.
- Merge Common.props files into .csproj
- Remove projects from solution
@tmilnthorp
Copy link
Collaborator Author

I think alternate #2 makes sense.

@tmilnthorp tmilnthorp merged commit b958303 into angularsen:v4 Nov 6, 2018
angularsen added a commit that referenced this pull request Nov 9, 2018
It was lost in a bad merge conflict handling.

Fixes:
commit b958303
Author: Tristan Milnthorp <[email protected]>
Date:   Tue Nov 6 11:12:52 2018 -0500

    Sign all packages and remove .Signed packages (#539)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants