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

Don't reference netstandard1.x packages #74199

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jun 28, 2024

... when building from source.

This was the only remaining project in roslyn that brought them in.

Contributes to dotnet/source-build#4482

... when building from source.

This was the only remaining project in roslyn that brought them in.
@ViktorHofer ViktorHofer requested a review from a team as a code owner June 28, 2024 07:54
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Interactive untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 28, 2024
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 28, 2024
@ViktorHofer ViktorHofer requested a review from a team as a code owner June 28, 2024 07:56
@jjonescz
Copy link
Contributor

@ViktorHofer source build is failing

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jun 28, 2024

@MichalStrehovsky just to double check, src/Scripting/Core/Microsoft.CodeAnalysis.Scripting.csproj shouldn't be referencing the old System.Runtime.Loader package on netstandard2.0 and we should convert AssemblyLoadContext usages to AssemblyLoader when targeting netfx or netstandard, right?

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky just to double check, src/Scripting/Core/Microsoft.CodeAnalysis.Scripting.csproj shouldn't be referencing the old System.Runtime.Loader package on netstandard2.0 and we should convert AssemblyLoadContext usages back to AssemblyLoader when targeting netfx or netstandard, right?

Yeah, I don't think the System.Runtime.Loader would do anything good on netfx. Cc @dotnet/appmodel

@ViktorHofer
Copy link
Member Author

What makes this problematic is that Microsoft.CodeAnalysis.Scripting dynamically instantiates either an AssemblyLoadContext or an AssemblyLoader:

public static AssemblyLoaderImpl Create(InteractiveAssemblyLoader loader)
{
if (CoreClrShim.AssemblyLoadContext.Type != null)
{
return CreateCoreImpl(loader);
}
else
{
return new DesktopAssemblyLoaderImpl(loader);
}
}

@ViktorHofer
Copy link
Member Author

cc @jaredpar @MichaelSimons @ericstj

The goal is to remove all netstandard1.x package dependencies when building from source (see dotnet/source-build#4482). Roslyn is one of the few remaining repos that bring a netstandard1.x package in. For roslyn it's only a single reference in a single place: System.Runtime.Loader/4.3.0 by Microsoft.CodeAnalysis.Scripting.csproj.

See my comments above. It's hard to get rid off that package as roslyn dynamically instantiates either an ALC or AssemblyLoader.

@ViktorHofer ViktorHofer marked this pull request as draft June 28, 2024 13:11
@jaredpar
Copy link
Member

@tmat, @arkalyanms FYI

@ericstj
Copy link
Member

ericstj commented Jun 28, 2024

For roslyn it's only a single reference in a single place: System.Runtime.Loader/4.3.0 by Microsoft.CodeAnalysis.Scripting.csproj.

As a workaround you could PackageDownload then raw-reference the file. That will avoid pulling the dependencies.

@ViktorHofer
Copy link
Member Author

As a workaround you could PackageDownload then raw-reference the file. That will avoid pulling the dependencies.

Yes, I considered that. The goal here is to remove all netstandard1.x packages. If we keep the System.Runtime.Loader reference then we would need to continue building it in SBRP including its dependencies.

@jaredpar
Copy link
Member

The compiler approaches this by representing assembly loading through an interface at the netstandard2.0 layer. Then we have our .NET Framework / Core implementations that are fed through by the host. The actual host is going to target Core or Framework so it can be definitive in what it references.

That model was designed into the compiler very early on though. Not sure if the scripting layer can take this approach or not.

@arkalyanms arkalyanms requested a review from tmat June 28, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interactive Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants