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

Does PolySharp support multitargeting? #48

Closed
sungam3r opened this issue Jan 16, 2023 · 11 comments
Closed

Does PolySharp support multitargeting? #48

sungam3r opened this issue Jan 16, 2023 · 11 comments
Labels
not an issue Something that's by design or not a real issue

Comments

@sungam3r
Copy link

I recently saw this package and decided to try.

Config:

<PackageReference Include="PolySharp" Version="1.10.0">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>

Output:
изображение

If I change reference to

 <PackageReference Include="PolySharp" Version="1.10.0" Condition="'$(TargetFramework)' != 'net5' AND '$(TargetFramework)' != 'net6' AND '$(TargetFramework)' != 'net7'">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>

then there are no errors.

I would like PolySharp does not require Conditional attribute on TFMs where all necessary attributes are present.

@sungam3r
Copy link
Author

@Sergio0694
Copy link
Owner

PolySharp already supports this (eg. I'm personally using already in several multi-targeted projects, like you can see here: Sergio0694/ComputeSharp#471). I'm not entirely sure what's going on here. Are you able to provide a minimal repro?

@Sergio0694 Sergio0694 added the needs more info Can't repro or unclear issue, needs further info label Jan 16, 2023
@Shane32
Copy link

Shane32 commented Jan 16, 2023

Maybe this has more to do with multiple projects rather than multiple targets. Is the injected code internal?

@Sergio0694
Copy link
Owner

The fact you're only getting the error for a single type is very suspect. As in, it's easy to see that PolySharp does indeed skip types that already exist: if that wasn't the case you'd be getting dozens of errors, not just one for IsExternalInit. The fact that that's the case makes me think it's possible you might have another generator that's also generating that polyfill, which ends up conflicting. Have you double checked in your analyzer nodes that this is not the case? 🤔

@Sergio0694
Copy link
Owner

"Is the injected code internal?"

@Shane32 it is by default, yes, unless you explicitly set the "PolySharpUsePublicAccessibilityForGeneratedTypes" property.

@Shane32
Copy link

Shane32 commented Jan 16, 2023

@Sergio0694 It may be due to the fact that the test project, where the build is failing, has references to two different projects both of which utilize InternalsVisibleTo back towards the test project... ??

@Sergio0694
Copy link
Owner

Ah, yeah that makes sense. If the test project is, say, targeting .NET 6 (which has the attribute), but is referencing a project on, say, the .NET Standard 2.0 project, that project will have IsExternalinit generated for it, but then InternalsVisibleTo will make that give visibility to the test project as well, which now has two versions of that attirbute... I guess that could be possible 🤔

@Shane32
Copy link

Shane32 commented Jan 16, 2023

Attempting to reproduce now.......

@Sergio0694
Copy link
Owner

Anyway if the issue is in a test project this definitely doesn't seem related to PolySharp per se. The library is correctly generating only the polyfills that are needed on each TFM, if then this specific repo has some weird configuration with test projects and InternalsVisibleTo settings caausing this issue in a separate project, this seems like a different issue 😅

Closing this for now, happy to reopen in case more info is provided that links this to an actual issue in PolySharp 🙂
I assume for that project, adding the condition on the PackageReference seems like an easy enough workaround at least.

@Sergio0694 Sergio0694 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 16, 2023
@Sergio0694 Sergio0694 added not an issue Something that's by design or not a real issue and removed needs more info Can't repro or unclear issue, needs further info labels Jan 16, 2023
@Shane32
Copy link

Shane32 commented Jan 16, 2023

FYI, below is a sample. Removing PolySharp from the console app, or setting the TFM for ClassLibrary1 to netstandard2.0;net5.0, or adding a condition to prevent PolySharp from being included on .NET 5+ "solves" the issue.

ClassLibrary1

<Project Sdk="Microsoft.NET.Sdk">

	<PropertyGroup>
		<TargetFramework>netstandard2.0</TargetFramework>
		<ImplicitUsings>enable</ImplicitUsings>
		<Nullable>enable</Nullable>
		<LangVersion>11</LangVersion>
	</PropertyGroup>

	<ItemGroup>
		<InternalsVisibleTo Include="ConsoleApp1" />
	</ItemGroup>

	<ItemGroup>
		<PackageReference Include="PolySharp" Version="1.10.0">
			<PrivateAssets>all</PrivateAssets>
			<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
		</PackageReference>
	</ItemGroup>

</Project>
namespace ClassLibrary1
{
    public class Class1
    {
    }
}

ConsoleApp1

<Project Sdk="Microsoft.NET.Sdk">

	<PropertyGroup>
		<OutputType>Exe</OutputType>
		<TargetFramework>net7.0</TargetFramework>
		<ImplicitUsings>enable</ImplicitUsings>
		<Nullable>enable</Nullable>
	</PropertyGroup>

	<ItemGroup>
		<ProjectReference Include="..\ClassLibrary1\ClassLibrary1.csproj" />
	</ItemGroup>

	<ItemGroup>
		<PackageReference Include="PolySharp" Version="1.10.0">
			<PrivateAssets>all</PrivateAssets>
			<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
		</PackageReference>
	</ItemGroup>

</Project>
// See https://aka.ms/new-console-template for more information
Console.WriteLine("Hello, World!");

@Nihlus
Copy link

Nihlus commented May 1, 2023

I've got the same issue with IsExternalInit, also in a test project that references another project in the same repository. It's likely to prevent the use of PolySharp as a replacement for IsExternalInit and Nullable in Remora.Sdk for the time being - see Remora/Remora.Results#10 for the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not an issue Something that's by design or not a real issue
Projects
None yet
Development

No branches or pull requests

4 participants