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

Using new C# 9.0 language feature causes the performance CI tests to fail #39081

Closed
steveharter opened this issue Jul 10, 2020 · 17 comments
Closed

Comments

@steveharter
Copy link
Member

When trying to use the new C# 9.0 'record' usage in tests the dotnet-runtime-perf leg failed with:
Serialization\ConstructorTests\ConstructorTests.Exceptions.cs(156,37): error CS0518: (NETCORE_ENGINEERING_TELEMETRY=Build) Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported

The test csproj did need to add <LangVersion>preview</LangVersion>, which seemed to work fine execpt for the performance leg:

#38959 (comment)

@ghost
Copy link

ghost commented Jul 10, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 10, 2020
@stephentoub
Copy link
Member

cc: @DrewScoggins

@DrewScoggins
Copy link
Member

I will take a look at this.

@DrewScoggins
Copy link
Member

DrewScoggins commented Jul 10, 2020

Is the link to the PR above the one where you are seeing the failure? Because when I look at that all of the CI is green.

EDIT: I see, that was taken out of the PR.

@safern safern removed the untriaged New issue has not been triaged by the area owner label Jul 10, 2020
@safern safern added this to the 5.0.0 milestone Jul 10, 2020
@DrewScoggins
Copy link
Member

So I investigated this and what happened was that the AzDO controller somehow got disconnected from tracking the Helix work items. You can see that in the failure on AzDO here, https://dev.azure.com/dnceng/public/_build/results?buildId=721665&view=logs&j=319274cb-4008-5c3f-98b0-4b6b2825e9cf. So I dug into the Kusto logs of Helix and for both of the "failed" jobs all of the workitems completed correctly. So there is no problem here, you can just add back that record type and I doubt it will cause an issue. This failure just seems like a transient issue in AzDO.

@safern
Copy link
Member

safern commented Jul 10, 2020

Yeah, I'm seeing the same behavior as @DrewScoggins. @steveharter if you don't want to block your PR on CI, could you merge and put up another PR to re-enable this and see if we can get a repro or if it was just an unrelated transient issue.

@steveharter
Copy link
Member Author

I'll re-add the record type and ping you two if there's an issue. Thanks

@steveharter
Copy link
Member Author

There is still an issue: #38959 (comment)

@steveharter
Copy link
Member Author

@jaredpar is this issue a duplicate of dotnet/roslyn#45510?

@safern
Copy link
Member

safern commented Jul 15, 2020

@jaredpar are there any plans on supporting the record type downlevel?

@jaredpar
Copy link
Member

@steveharter

Essentially yes it's a dupe

@safern

The issue here isn't so much downlevel support as we're missing a type required for code geneartion of the feature. The type is just a marker type (used for some modreq). If the down level frameworks have this type defined then the compiler can emit record types properly.

Going back in time it's very similar to extension methods. The type necessary to emit extension methods, ExtensionAttribute, was added in .NET 3.5. It was still possible to use them in 2.0 though, u just had to define the type yourself in the code.

@safern
Copy link
Member

safern commented Jul 15, 2020

I see.

@steveharter if you wanted those tests to work, then we can just define the type on the test project for net472 then, right?

@steveharter
Copy link
Member Author

then we can just define the type on the test project for net472 then, right?

Yes essentially. Currently I just have an #if but could have moved the tests to a separate .cs file along with a conditional include in the csproj. The latter approach would be best if this is going to be a long-term issue.

@safern
Copy link
Member

safern commented Jul 15, 2020

It seems like that is how it is going to be based on:

The issue here isn't so much downlevel support as we're missing a type required for code geneartion of the feature. The type is just a marker type (used for some modreq). If the down level frameworks have this type defined then the compiler can emit record types properly.
Going back in time it's very similar to extension methods. The type necessary to emit extension methods, ExtensionAttribute, was added in .NET 3.5. It was still possible to use them in 2.0 though, u just had to define the type yourself in the code.

@safern
Copy link
Member

safern commented Jul 15, 2020

@steveharter maybe we should either close this issue or fix it as suggested above. Up to you what direction we want to go here.

@steveharter
Copy link
Member Author

So I should add this type to the test project for #if !NETCOREAPP:

namespace System.Runtime.CompilerServices
{
    public sealed class IsExternalInit
    {
    }
}

?

@safern
Copy link
Member

safern commented Jul 15, 2020

Yeah, I think so. We should just put it in a separate file though and conditionally include it to the Compile items based on the TFM.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
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

6 participants