Stop harvesting S.Security.Cryptography.Pkcs #52045
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsThe removed configurations (netstandard1.3, netcoreapp2.1, net46) Dropping these assets as the minimum supported set of platforms are ones Contributes to #47530 @ericstj unfortunately there are still errors which I wasn't able to resolve. I might need your help again:
|
ViktorHofer
changed the title
25 tasks
|
All the errors are due to The supportedframework metadata is applying max version from all TFMs of referenced project and maps that to all frameworks listed. Since some of those have a different version it’s incorrect (similar to problem @Anipik faces in servicing when only netfx increments). |
So in such situation we need to disable the support check? |
|
And presumably we also need to downgrade netstandard2.0's AssemblyVersion to the same as the harvested netstandard2.0 in the package which is 4.0.3.1? I had it like that before but reverted the change beause of the errors: |
|
Can you remove the fixed assembly version here? Not sure what the history is but that’s a preferable solution. I’d be surprised if we had a good reason to fix assemblyversion on any of the remaining nuget packages. @Anipik might have background here. |
|
I don't have the details here but does the comment indicate that the APIs are locked and the assembly version should stay the same? |
|
OK, the types in that assembly aren't exposed in an assembly with the same name on .NET Framework, instead those live in System.Security. So I don't see any concern with updating the AssemblyVersion. Will do so. |
|
OK I take that back. Based on this comment it sounds like we can't bump netstandard's AssemblyVersion: dotnet/corefx#35429 (comment). |
Have a look at what the package did at that time: https://github.com/bartonjs/corefx/blob/1f9dfefe911486667c9c394444d2d193affe3a01/src/System.Security.Cryptography.Pkcs/pkg/System.Security.Cryptography.Pkcs.pkgproj#L9-L12 It was harvesting reference assemblies, but then live-building the implementations: That was what was breaking validation at the time: it didn't like newer minor version for impl since it was trying to make sure people exposed the right version of the reference assembly. Contrast that to now: you're live building everything. Would be nice to simplify this. I don't think we are trying to represent the different API on different TargetFrameworks in assembly versions any more, we're already not doing this in other packages (EG: the drawing case we just looked at). Double-check that surface are in your new netstandard2.0 ref build matches what was previously harvested. Then I'd say move everything to 6.0.0.0. |
ViktorHofer
force-pushed
the
PkcsHarvesting
branch
from
e5444ac to
fc262bd
Compare
|
That does make sense @ericstj, thanks for the help here again. I still hit validation error with the netstandard2.1 assembly version when targeting Xamarin and Mono tfms. Presumably because I'm bumping the assembly version there? Would I want to suppress there errors? Looked further into this and the errors happen because of the harvested assets having the old assembly versions and package validation complaining about the bump. I decided to disable the support validation. |
|
The netstandard2.0 asm diff looks fine. The only observable difference is the addition of System.Security.Cryptography.Pkcs {
+ namespace System.Diagnostics.CodeAnalysis {
+ internal sealed class AllowNullAttribute : Attribute
+ internal sealed class DisallowNullAttribute : Attribute
+ internal sealed class DoesNotReturnAttribute : Attribute
+ internal sealed class DoesNotReturnIfAttribute : Attribute
+ internal sealed class MaybeNullAttribute : Attribute
+ internal sealed class MaybeNullWhenAttribute : Attribute
+ internal sealed class MemberNotNullAttribute : Attribute
+ internal sealed class MemberNotNullWhenAttribute : Attribute
+ internal sealed class NotNullAttribute : Attribute
+ internal sealed class NotNullIfNotNullAttribute : Attribute
+ internal sealed class NotNullWhenAttribute : Attribute
+ }
- namespace System.Runtime.CompilerServices {
{
- internal class __BlockReflectionAttribute : Attribute
- }
+ namespace System.Runtime.Versioning {
+ internal abstract class OSPlatformAttribute : Attribute
+ internal sealed class SupportedOSPlatformAttribute : OSPlatformAttribute
+ internal sealed class TargetPlatformAttribute : OSPlatformAttribute
+ internal sealed class UnsupportedOSPlatformAttribute : OSPlatformAttribute
+ }
namespace System.Security.Cryptography {
public sealed class CryptographicAttributeObject {
- public CryptographicAttributeObject(Oid oid, AsnEncodedDataCollection values);
+ public CryptographicAttributeObject(Oid oid, AsnEncodedDataCollection? values);
}
public sealed class CryptographicAttributeObjectCollection : ICollection, IEnumerable {
+ public bool IsSynchronized { get; }
+ public object SyncRoot { get; }
- bool System.Collections.ICollection.IsSynchronized { get; }
- object System.Collections.ICollection.SyncRoot { get; }
- void System.Collections.ICollection.CopyTo(Array array, int index);
+ void ICollection.CopyTo(Array array, int index);
- IEnumerator System.Collections.IEnumerable.GetEnumerator();
+ IEnumerator IEnumerable.GetEnumerator();
}
public sealed class CryptographicAttributeObjectEnumerator : IEnumerator {
- object System.Collections.IEnumerator.Current { get; }
+ object IEnumerator.Current { get; }
}
}
namespace System.Security.Cryptography.Pkcs {
public sealed class AlgorithmIdentifier {
+ public byte[] Parameters { get; set; }
}
public sealed class CmsRecipientCollection : ICollection, IEnumerable {
+ public bool IsSynchronized { get; }
+ public object SyncRoot { get; }
- bool System.Collections.ICollection.IsSynchronized { get; }
- object System.Collections.ICollection.SyncRoot { get; }
- IEnumerator System.Collections.IEnumerable.GetEnumerator();
+ IEnumerator IEnumerable.GetEnumerator();
}
public sealed class CmsRecipientEnumerator : IEnumerator {
- object System.Collections.IEnumerator.Current { get; }
+ object IEnumerator.Current { get; }
}
public sealed class CmsSigner {
- public CmsSigner(SubjectIdentifierType signerIdentifierType, X509Certificate2 certificate);
+ public CmsSigner(SubjectIdentifierType signerIdentifierType, X509Certificate2? certificate);
- public CmsSigner(X509Certificate2 certificate);
+ public CmsSigner(X509Certificate2? certificate);
- public X509Certificate2 Certificate { get; set; }
+ public X509Certificate2? Certificate { get; set; }
}
public sealed class KeyAgreeRecipientInfo : RecipientInfo {
- public CryptographicAttributeObject OtherKeyAttribute { get; }
+ public CryptographicAttributeObject? OtherKeyAttribute { get; }
}
public class Pkcs9AttributeObject : AsnEncodedData {
- public Oid Oid { get; }
+ public Oid? Oid { get; }
}
public sealed class RecipientInfoCollection : ICollection, IEnumerable {
+ public bool IsSynchronized { get; }
+ public object SyncRoot { get; }
- bool System.Collections.ICollection.IsSynchronized { get; }
- object System.Collections.ICollection.SyncRoot { get; }
- IEnumerator System.Collections.IEnumerable.GetEnumerator();
+ IEnumerator IEnumerable.GetEnumerator();
}
public sealed class RecipientInfoEnumerator : IEnumerator {
- object System.Collections.IEnumerator.Current { get; }
+ object IEnumerator.Current { get; }
}
public sealed class SignerInfo {
- private SignerInfo();
+ internal SignerInfo();
- public X509Certificate2 Certificate { get; }
+ public X509Certificate2? Certificate { get; }
}
public sealed class SignerInfoCollection : ICollection, IEnumerable {
- IEnumerator System.Collections.IEnumerable.GetEnumerator();
+ IEnumerator IEnumerable.GetEnumerator();
}
public sealed class SignerInfoEnumerator : IEnumerator {
- private SignerInfoEnumerator();
+ internal SignerInfoEnumerator();
- object System.Collections.IEnumerator.Current { get; }
+ object IEnumerator.Current { get; }
}
}
namespace System.Security.Cryptography.Xml {
public struct X509IssuerSerial {
+ private int _dummyPrimitive;
}
}
} |
The removed configurations (netstandard1.3, netcoreapp2.1, net46) of the touched packages aren't built anymore. Instead the already built matching binaries from the latest available packages are redistributed when packaging these libraries. Dropping these assets as the minimum supported set of platforms are ones that support netstandard2.0 and are still in-support. As an example, also dropping the netcoreapp2.1 asset which isn't supported anymore and adding a target to prevent the package being used for netcoreapp2.x. For .NET Framework, there's still a net461 configuration present to avoid binding redirect issues. Contributes to dotnet#47530
ViktorHofer
force-pushed
the
PkcsHarvesting
branch
from
fc262bd to
affb212
Compare
ericstj
reviewed
May 3, 2021
| @@ -640,9 +638,6 @@ | |||
| <PackageReference Include="System.Buffers" Version="$(SystemBuffersVersion)" /> | |||
| <PackageReference Include="System.Memory" Version="$(SystemMemoryVersion)" /> | |||
| <ProjectReference Include="$(LibrariesProjectRoot)System.Runtime.CompilerServices.Unsafe\src\System.Runtime.CompilerServices.Unsafe.ilproj" /> | |||
| <!-- Ref assembly for netstandard2.0 isn't live built anymore. --> | |||
| <PackageDownload Include="$(MSBuildProjectName)" Version="[$(SystemSecurityCryptographyPkcsVersion)]" /> | |||
| <ResolvedMatchingContract Include="$(NuGetPackageRoot)$(MSBuildProjectName.ToLowerInvariant())\$(SystemSecurityCryptographyPkcsVersion)\ref\$(TargetFramework)\$(MSBuildProjectName).dll" /> | |||
There was a problem hiding this comment.
Nit: I was looking at when this was introduced and found
Might be an opportunity for further cleanup, removing SystemSecurityCryptographyPkcsVersion entirely. Not necessarily as part of this PR.
There was a problem hiding this comment.
I only grabbed for "$(ProjectName).csproj" but not for "$(ProjectName)" which is why I missed this dependent project. Thanks for the great feedback. I submitted #52239 to address this.
ericstj
approved these changes
May 3, 2021
ViktorHofer
added
the
breaking-change
ghost
added
the
needs-breaking-change-doc-created
ghost
locked as resolved and limited conversation to collaborators
ViktorHofer
removed
the
needs-breaking-change-doc-created
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The removed configurations (netstandard1.3, netcoreapp2.1, net46)
of the touched packages aren't built anymore. Instead
the already built matching binaries from the latest available packages
are redistributed when packaging these libraries.
Dropping these assets as the minimum supported set of platforms are ones
that support netstandard2.0 and are still in-support. As an example,
also dropping the netcoreapp2.1 asset which isn't supported anymore and
adding a target to prevent the package being used for netcoreapp2.x.
For .NET Framework, there's still a net461
configuration present to avoid binding redirect issues.
Contributes to #47530
@ericstj unfortunately there are still errors which I wasn't able to resolve. I might need your help again: