-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add Base64Url support for netstandard 2.0 #103617
Conversation
Note regarding the
|
Note regarding the
|
src/libraries/Microsoft.Bcl.Memory/src/Microsoft.Bcl.Memory.csproj
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Bcl.Memory/src/Microsoft.Bcl.Memory.csproj
Outdated
Show resolved
Hide resolved
@buyaa-n I tried my feedback out in a code space. Here's the patch that you can directly apply: From 2b35645c7eee21dfdbdd35855d709c9def195097 Mon Sep 17 00:00:00 2001
From: Viktor Hofer <viktor.hofer@microsoft.com>
Date: Tue, 18 Jun 2024 16:17:54 +0000
Subject: [PATCH] Enable compiler doc generation and simplify forwards
---
.../src/Microsoft.Bcl.Memory.csproj | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/libraries/Microsoft.Bcl.Memory/src/Microsoft.Bcl.Memory.csproj b/src/libraries/Microsoft.Bcl.Memory/src/Microsoft.Bcl.Memory.csproj
index aac0fba3d95..fa7bc6e2c33 100644
--- a/src/libraries/Microsoft.Bcl.Memory/src/Microsoft.Bcl.Memory.csproj
+++ b/src/libraries/Microsoft.Bcl.Memory/src/Microsoft.Bcl.Memory.csproj
@@ -3,7 +3,6 @@
<PropertyGroup>
<TargetFrameworks>netstandard2.0;$(NetFrameworkMinimum);$(NetCoreAppCurrent)</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
- <UseCompilerGeneratedDocXmlFile>false</UseCompilerGeneratedDocXmlFile>
<IsPackable>true</IsPackable>
<!-- Disabling baseline validation since this is a brand new package.
Once this package has shipped a stable version, the following line
@@ -19,17 +18,20 @@
<!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
<PropertyGroup>
- <IsPartialFacadeAssembly Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp'">true</IsPartialFacadeAssembly>
- <OmitResources Condition="'$(IsPartialFacadeAssembly)' == 'true'">true</OmitResources>
+ <OmitResources Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp'">true</OmitResources>
</PropertyGroup>
- <ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true'">
+ <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
+ <Compile Include="..\ref\Microsoft.Bcl.Memory.Forwards.cs" />
+ </ItemGroup>
+
+ <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
<Compile Include="System\Buffers\Text\Base64UrlDecoder.cs" />
<Compile Include="System\Buffers\Text\Base64UrlEncoder.cs" />
<Compile Include="System\Buffers\Text\Base64UrlValidator.cs" />
</ItemGroup>
- <ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true'">
+ <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
<PackageReference Include="System.Memory" Version="$(SystemMemoryVersion)" />
</ItemGroup>
|
The allconfigurations leg is failing because the netstandard2.0 version of Base64Url has the ClsCompliant(false) attribute on it but the net9.0 version doesn't. If that's intentional, create a suppression file. The error explains how to do that ( |
Thanks @ViktorHofer, couldn't you apply the patch yourself? I am not really sure how to apply the patch |
No, I don't have write permissions to your fork. You can either enable perms or apply the patch yourself (if |
For the PR
I tried that and |
@ViktorHofer I added you as collaborator, please accept and apply your patch |
The removal of the ClsCompliant(false) attribute is non-breaking, right? I'm 99% sure it isn't but just want to double check. |
Weird there were ClsCompliant warning on |
Co-authored-by: Stephen Toub <stoub@microsoft.com>
…e64UrlDecoder.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlDecoder.cs
Outdated
Show resolved
Hide resolved
// This should never overflow since destLength here is less than int.MaxValue / 4 * 3 (i.e. 1610612733) | ||
// Therefore, (destLength / 3) * 4 will always be less than 2147483641 | ||
Debug.Assert(destLength < (int.MaxValue / 4 * 3)); | ||
#if NETCOREAPP |
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.
With the exception of the code in these ifdefs, and with calls being prefixed with decoder.
, this whole file is an exact copy of what was in the other file, right? Or are there other changes?
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.
Exactly as you said,, only changes are if-defs for netsandard, and type parameter to decoder
no new changes. Well, there is small bugfix that related to the new tests added, nothing else new
Basically all internal members from Base64Decoder
and Base64Encoder
moved in this Base64Herlper
type to avoid conflict referencing Base64
from netstandard project between .NET 9 and netstandard versions.
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.
Well, there is small bugfix that related to the new tests added, nothing else new
What is that fix?
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.
I was fixed the bug found with Base64Url fuzzing only for byte overloads, the same bug left for char overloads.
The diff is here 73acb51
Add Base64Url package that supports netstandard 2.0 and .NET framework
Pretty much same implementation excluding the parts that not supported in netstandard like:
Span.IndexOfAnyExcept(SearchValues)
support so validation APIs logic differs around that are, but main logic is sameFixes #1658