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

Update target framework version in toolsetinfo for NET 8. #29316

Merged
merged 7 commits into from
Dec 13, 2022

Conversation

nagilson
Copy link
Member

It would be ideal if this could be done automatically, though per Marc it could make the migration to 9 harder so probably not.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@marcpopMSFT
Copy link
Member

You may want nexttargetframework to stay as 8.0. Jason added that for the transition from 7 to 8.0 as we had some tests that required 7 still and some that required 8 already. I don't think those tests will work targeting 9. The alternative is to move all tests that use next target framework currently back to current with this PR.

I like that alternative better.

@nagilson
Copy link
Member Author

@marcpopMSFT Pretty sure that's already been done, there are no references to either of the nextTFM properties.
image

@dsplaisted
Copy link
Member

dsplaisted commented Dec 1, 2022

I don't think that "Find all references" search is working. I see references still here:

[InlineData(true, true, ToolsetInfo.NextTargetFramework)]
[InlineData(true, false, ToolsetInfo.NextTargetFramework)]
[InlineData(false, false, ToolsetInfo.NextTargetFramework)]

new object[] { ToolsetInfo.NextTargetFramework }

@v-wuzhai
Copy link
Member

v-wuzhai commented Dec 1, 2022

The NextTargetFramework constant is also used in #29158.

@nagilson
Copy link
Member Author

nagilson commented Dec 13, 2022

~~ @dotnet/templating-engine-maintainers PTAL at the failure:
Microsoft.DotNet.Cli.New.IntegrationTests.CommonTemplatesTests.FeaturesSupport(name: \"console\", buildPass: True, framework: \"net7.0\", langVersion: null, langVersionUnsupported: False, language: null, supportsNullable: True, supportsTopLevel: True, forceDisableTopLevel: False, supportsImplicitUsings: True, supportsFileScopedNs: True) with the new ToolsetInfo.CurrentTargetFramework.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=110027&view=ms.vss-test-web.build-test-results-tab
~~

JK it's been fixed.

@nagilson
Copy link
Member Author

@dotnet/dotnet-cli PTAL

@baronfel
Copy link
Member

I'm glad that we caught some build-logic updates (warning levels) with this bump :) How concerned should I be that it looks like we're losing a lot of coverage on net7.0 TFMs on tests?

@nagilson
Copy link
Member Author

nagilson commented Dec 13, 2022

I am not too concerned since they are still running in the 7x branches and theoretically any gap in 8 and 7 should be specifically tested with 7 and 8. (I'm not saying they are, but stylistically it would be the correct thing to do and it's what I've been doing for 8.0 breaking changes.)

@baronfel
Copy link
Member

@nagilson impeccable response, you are of course correct on all counts. Thank you :)

@nagilson
Copy link
Member Author

Lol! I have gotten better at covering my bases.

@nagilson nagilson merged commit 791f5ee into dotnet:main Dec 13, 2022
@marcpopMSFT
Copy link
Member

+1 to what Noah said. We should probably identify a specific set of tests that target all supported runtimes as a baseline but everything else we should roll forward each release.

@sbomer sbomer mentioned this pull request Dec 20, 2022
sbomer added a commit that referenced this pull request Dec 21, 2022
The "net7.0" TFM was dropped in the move to "net8.0" in #29316.
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.

6 participants