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

Fix netcoreapp3.0 and older builds #2352

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

timcassell
Copy link
Collaborator

Fixes #2333

@timcassell timcassell marked this pull request as draft July 4, 2023 15:22
@timcassell timcassell force-pushed the fix-netcoreapp3.0 branch 2 times, most recently from 937e3ff to ecaff2c Compare July 4, 2023 16:20
@timcassell timcassell marked this pull request as ready for review July 4, 2023 17:16
@timcassell timcassell marked this pull request as draft July 5, 2023 14:42
@timcassell
Copy link
Collaborator Author

@AndreyAkinshin May need to update permissions for that report workflow to succeed. actions/first-interaction#10 (comment)

@AndreyAkinshin
Copy link
Member

@timcassell
Copy link
Collaborator Author

Hm, then I'm not sure why it's failing with HttpError: Resource not accessible by integration. It passed on my fork.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

It LGTM, please feel free to mark it as ready and then I am going to simply merge it.

thank you @timcassell !

<PackageReference Include="System.Management" Version="5.0.0" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.2.0" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetFramework)' == 'net6.0' ">
Copy link
Member

Choose a reason for hiding this comment

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

nit: in the future we might add net7.0, net8.0 and other targets, so we could just change the condition to "everything that is not .NET Standard 2.0"

Suggested change
<ItemGroup Condition=" '$(TargetFramework)' == 'net6.0' ">
<ItemGroup Condition=" '$(TargetFramework)' != 'netstandard2.0' ">

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather be explicit about that since updating the nuget packages in the future could also break net6.0 etc when their support is dropped.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather be explicit about that since updating the nuget packages in the future could also break net6.0 etc when their support is dropped.

Good point!

@timcassell timcassell marked this pull request as ready for review July 7, 2023 10:44
<PackageReference Include="System.Management" Version="5.0.0" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.2.0" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetFramework)' == 'net6.0' ">
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather be explicit about that since updating the nuget packages in the future could also break net6.0 etc when their support is dropped.

Good point!

@adamsitnik adamsitnik merged commit 2b8bc88 into dotnet:master Jul 7, 2023
8 of 9 checks passed
@AndreyAkinshin AndreyAkinshin added this to the v0.13.6 milestone Jul 7, 2023
@timcassell timcassell deleted the fix-netcoreapp3.0 branch July 7, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot benchmark netcoreapp3.0
3 participants