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

Ensure no component embed the Index/Range source code and reference Microsoft.Bcl.Memory package instead #104308

Closed
tarekgh opened this issue Jul 2, 2024 · 5 comments

Comments

@tarekgh
Copy link
Member

tarekgh commented Jul 2, 2024

Description

We have poly filled the Index and Range types to make them work on ns2.0 and netfx. This issue is tracking to ensure all components we build in .NET shouldn't carry its own version of these types and instead should be referencing Microsoft.Bcl.Memory package.

Searching the sources, looks only ComInterfaceGenerator.csproj is using the Index source file. To remove this dependency, we will need to have this project depend on Microsoft.Bcl.Memory and ensure Microsoft.Bcl.Memory library is included in microsoft.netcore.app.ref pack too under the analyzers\dotnet\cs folder. Unfortunately, the current infrastructure does not allow to bin pace such dependency.

Here are the things need to be done:

  • Remove compiling the Index source code in
    <Compile Include="$(CoreLibSharedDir)System\Index.cs" Link="Common\System\Index.cs" />
  • Let ComInterfaceGenerator.csproj reference the project of Microsoft.Bcl.Memory.
  • Delete the lines
    <ItemGroup Condition="'$(IncludeIndexRangeTypes)' == 'true' and '$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
    <Compile Include="$(CoreLibSharedDir)System\Index.cs" Link="System\Index.cs" />
    <Compile Include="$(CoreLibSharedDir)System\Range.cs" Link="System\Range.cs" />
    <Compile Include="$(CoreLibSharedDir)System\Numerics\Hashing.cs" Link="System\Numerics\Hashing.cs" />
    </ItemGroup>
  • Ensure when building Microsoft.Bcl.Memory is bin placed inside the microsoft.netcore.app.ref package under analyzers\dotnet\cs folder and next to the ComInterfaceGenerator lib.
  • Edit Index and Range to remove the #if there and only keep public specifier.
  • edit Microsoft.Bcl.Memory.csproj from library src folder to remove the line <DefineConstants>MICROSOFT_BCL_MEMORY;$(DefineConstants)</DefineConstants>
  • Build the whole tree and validate tests of the com interface and Microsoft.Bcl.Memory pass and the package microsoft.netcore.app.ref include Microsoft.Bcl.Memory library.

Reproduction Steps

Com interface source gen is including the Index.cs in the project.

Expected behavior

No component in .NET is embedding the Index/Range sources.

Actual behavior

Com interface source gen carries a copy of the Index type code.

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 2, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 2, 2024
@tarekgh tarekgh added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 2, 2024
@tarekgh tarekgh added this to the 9.0.0 milestone Jul 2, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jul 2, 2024
@tarekgh
Copy link
Member Author

tarekgh commented Jul 2, 2024

@ViktorHofer
Copy link
Member

Please also see my concerns that I shared here.

@ericstj
Copy link
Member

ericstj commented Jul 8, 2024

I don't think we should be taking dependencies in our source generators. Analyzers don't have a great system for distributing dependencies - no way to specify those to the compiler, nor control loading - also no way to use the framework version when it exists. In cases of our analyzers it's better to share source.

@ViktorHofer
Copy link
Member

Closing as by design. We don't want source generator assemblies to take dependencies on other locally built assemblies for the above reasons (a contract system / targeting pack for compiler plugins don't exist today).

@ViktorHofer ViktorHofer closed this as not planned Won't fix, can't repro, duplicate, stale Jul 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants