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

Add Base64Url support for netstandard 2.0 #103617

Merged
merged 12 commits into from
Jun 24, 2024
Merged

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Jun 18, 2024

Add Base64Url package that supports netstandard 2.0 and .NET framework

Pretty much same implementation excluding the parts that not supported in netstandard like:

  • No vectorization, though the serial part of the implementation kept as is
  • No static abstract interface, therefore, no code sharing between Span, Span overloads
  • No Span.IndexOfAnyExcept(SearchValues) support so validation APIs logic differs around that are, but main logic is same
  • No Math.DivRem available

Fixes #1658

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ViktorHofer
Copy link
Member

@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>

@ViktorHofer
Copy link
Member

ViktorHofer commented Jun 18, 2024

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 (./dotnet.sh pack src/libraries/Microsoft.Bcl.Memory/src/Microsoft.Bcl.Memory.csproj /p:ApiCompatGenerateSuppressionFile=true).

@buyaa-n
Copy link
Member Author

buyaa-n commented Jun 18, 2024

@buyaa-n I tried my feedback out in a code space. Here's the patch that you can directly apply:

Thanks @ViktorHofer, couldn't you apply the patch yourself? I am not really sure how to apply the patch

@ViktorHofer
Copy link
Member

No, I don't have write permissions to your fork. You can either enable perms or apply the patch yourself (if git apply ... doesn't work, feel free to manually update the file).

@buyaa-n
Copy link
Member Author

buyaa-n commented Jun 18, 2024

No, I don't have write permissions to your fork. You can either enable perms

For the PR Allow edits and access to secrets by maintainers checkbox is checked, I will try to enable perms on my fork

apply the patch yourself (if git apply ... doesn't work, feel free to manually update the file).

I tried that and error: corrupt patch at line 45, I did not change/add anything, I guess it is from Linux/windows line endings

@buyaa-n
Copy link
Member Author

buyaa-n commented Jun 18, 2024

No, I don't have write permissions to your fork. You can either enable perms

@ViktorHofer I added you as collaborator, please accept and apply your patch

@ViktorHofer
Copy link
Member

The removal of the ClsCompliant(false) attribute is non-breaking, right? I'm 99% sure it isn't but just want to double check.

@buyaa-n
Copy link
Member Author

buyaa-n commented Jun 18, 2024

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 uint ushort types that is why I added the attribute, but somehow now I don't see the warning when removing the attribute, now removed the attribute and suppression

buyaa-n and others added 2 commits June 19, 2024 16:30
Co-authored-by: Stephen Toub <stoub@microsoft.com>
…e64UrlDecoder.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
// 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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Base 64 URL
4 participants