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

Remove MS.CA.Test.Resources.Proprietary PackageReference #75037

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

jaredpar
Copy link
Member

This package is the last remnants of the roslyn-internal repository. It was still holding onto a few test assets that never got ported into the open but were available through the NuPkg reference.

This package is becoming a problem with component governance as it is a netstandard1.3 package and hence brings in a lot of vulnerable components.

Considered upgrading this package to netstandard2.0 but that would mean un-archiving the roslyn-internal repo, finding a place to put the source code and setting up a new official pipeline for the package. That is a pretty high price. Instead I decided to finish the work that I should've completed a number of years ago.

Yes this does add a few more .dll into our git tree. These are files that haven't changed in 10+ years so versioning won't be an issue. They were all brought down already for restore hence it's not new files.

This package is the last remnants of the roslyn-internal repository. It
was still holding onto a few test assets that never got ported into the
open but were available through the NuPkg reference.

This package is becoming a problem with component governance as it
is a `netstandard1.3` package and hence brings in a lot of vulnerable
components.

Considered upgrading this package to `netstandard2.0` but that would
mean un-archiving the roslyn-internal repo, finding a place to put the
source code and setting up a new official pipeline for the package. That
is a pretty high price. Instead I decided to finish the work that
I should've completed a number of years ago.

Yes this does add a few more .dll into our git tree. These are files
that haven't changed in 10+ years so versioning won't be an issue. They
were all brought down already for restore hence it's not new files.
@jaredpar jaredpar requested review from a team as code owners September 10, 2024 04:50
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 10, 2024
/// to know that two mscorlib with different public key tokens need to be treated as the
/// identicial. The assemblies produced here have the same identity of those that come
/// from silverlight but without necessarily the same type contents.
/// </summary>
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 went back and forth on this decision several times. The Silverlight references are only used right now to test assembly unification policies (that I could see). Rather than the overhead of bringing in more .dll files decided to just dynamically build the same assemblies that would let us test unification policy.

@@ -85,11 +86,16 @@
<Content Include="NetFX\aacorlib\Key.snk" />
<EmbeddedResource Include="NetFX\Minimal\minasync.dll" />
<EmbeddedResource Include="NetFX\Minimal\mincorlib.dll" />
<EmbeddedResource Include="NetFX\PortableProfile7\mscorlib.dll" />
<EmbeddedResource Include="NetFX\PortableProfile7\System.Runtime.dll" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attempted to add a portable7 variant to Basic.Reference.Assemblies but these aren't available through NuPkg anywhere. Decided to just use them directly as resources.

<EmbeddedResource Include="NetFX\PortableProfile7\mscorlib.dll" />
<EmbeddedResource Include="NetFX\PortableProfile7\System.Runtime.dll" />
<EmbeddedResource Include="NetFX\WinRt\System.Runtime.WindowsRuntime.dll" />
<EmbeddedResource Include="NetFX\WinRt\System.Runtime.WindowsRuntime.UI.Xaml.dll" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two references are available as NuPkg and in theory could be added to the extra sections of Basic.Reference.Assemblies.Net461. Unfortunately, there is a substantial difference between how idiomatic net461 WinRt projects build using these references and how our tests build. The tests do not use the standard reference versions of these DLLs and using the standard references causes the tests to break. Decided to keep the existing DLLs to maintain test behavior.

@@ -15,6 +15,7 @@
</PropertyGroup>
<ItemGroup Label="Project References">
<ProjectReference Include="..\..\..\Core\Portable\Microsoft.CodeAnalysis.csproj" />
<PackageReference Include="stdole" IncludeAssets="none" PrivateAssets="all" GeneratePathProperty="true" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a .NET framework NuPkg hence redist directly within our repo is a bit suspect. Decided on the embed the NuPkg resource approach.

@jaredpar jaredpar requested a review from a team as a code owner September 10, 2024 13:40
private static readonly Lazy<MetadataReference> s_mscorlibRefPortable = new Lazy<MetadataReference>(
() => AssemblyMetadata.CreateFromImage(ProprietaryTestResources.v4_0_30319.mscorlib_portable).GetReference(display: "mscorlib.v4_0_30319.portable.dll"),
LazyThreadSafetyMode.PublicationOnly);
public static MetadataReference MscorlibRefPortable => s_mscorlibRefPortable.Value;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reference was primarily used for situations in the compiler where we needed a System.String type that lacked the GetEnumerator method. This setup for String is also present in the PortableProfile7 API (though corelib is System.Runtime.dll there). Moved compiler tests over to use that.

This was used in the symbol finder tests in the IDE but that doesn't appear to be anything specific to the portable library. Instead it was used because the symbol finder tests need to deal with retargeting between different core libraries. Flipped those to mostly use net40 and net461 mscorlib.

https://vstfdevdiv.corp.microsoft.com/DevDiv2/DevDiv/_workitems/edit/667616

@jaredpar jaredpar marked this pull request as draft September 10, 2024 20:15
@jaredpar jaredpar marked this pull request as ready for review September 10, 2024 20:41
@jaredpar
Copy link
Member Author

@dotnet/roslyn-compiler PTAL


return ImmutableCollectionsMarshal.AsImmutableArray(bytes);

byte parseByte(ReadOnlySpan<char> input, NumberStyles numberStyle)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on this in the audit PR

{
const string corlibExtraCode = """
using System;
using System.Reflection;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these usings look unnecessary. Maybe adding VerifyDiagnostics to the compilations we create here would catch that?

@jaredpar jaredpar merged commit 6de8947 into dotnet:main Sep 11, 2024
28 checks passed
@jaredpar jaredpar deleted the prop branch September 11, 2024 14:38
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Sep 11, 2024
333fred added a commit to 333fred/roslyn that referenced this pull request Sep 13, 2024
* upstream/main: (45 commits)
  Adjust an assert in EmitArgument (dotnet#75067)
  Switch to new VMR control set (dotnet#75056)
  Disallow ref assignment to ternary or another ref assignment (dotnet#75076)
  Move some emit tests from Emit2 to Emit3 to avoid hitting UserString heap limit (dotnet#75091)
  BindAttributeCore - use proper binder to avoid an attribute binding cycle (dotnet#75060)
  Remove buggy IsPublic method from TypeAttributesExtensions (dotnet#75081)
  Include initial filter node when searching for nodes to order modifiers
  Remove additional Gitter link (dotnet#75086)
  Remove newlines between test run information sections
  Log messages for Test Results
  Fix stack adjustment when emitting stackalloc (dotnet#75042)
  Lock translation of strings used to demonstrate identifier naming styles
  docs: Correct SDK version in documentation to match global.json (dotnet#75038)
  Add a test observing lack of an issue. (dotnet#75057)
  Fix preview refresh on selection for enum flags checkboxes
  Semantic snippets: handle case with inline statement snippets before member access expression (dotnet#74966)
  Configure release/vscode branch for nuget publishing
  Remove MS.CA.Test.Resources.Proprietary PackageReference (dotnet#75037)
  Allow suppressing nullability warnings in more ref scenarios (dotnet#74498)
  Update dependencies from https://github.com/dotnet/arcade build 20240909.6 (dotnet#75040)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants