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

Reverting change in source generators that require Roslyn 4.4 until we have a VS version with the compiler fix to consume them. #75490

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

joperezr
Copy link
Member

When #74822 got merged we realized that the compiler that is now required for using the source generators is not yet present in any VS preview version yet (or a functioning SDK) so we are instead going to wait till after RC1 and 17.4 Preview 2 ships to make this change. Otherwise, we would need to disable all of the SDK tests that run against VS MSBuild, which is not worth the risk of doing.

cc: @jkoritzinsky @danmoseley @jaredpar @CyrusNajmabadi

…e have a VS version with the compiler fix to consume them.
@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.

@ghost ghost assigned joperezr Sep 12, 2022
@joperezr
Copy link
Member Author

@danmoseley we would appreciate a sign off so we can merge and unblock the ingestion from runtime to installer. As the description says, we will do this once RC1 and 17.4 P2 ships.

@danmoseley
Copy link
Member

approved (of course)

Do I understand correctly that with this reverted, we are using the polyfill. So we don't expect a perf issue, it's clumsy but shippable if we must -- is that right?

@joperezr
Copy link
Member Author

Do I understand correctly that with this reverted, we are using the polyfill

Yes, this is a direct revert, and this is now falling back to use polyfill again. We weren't expecting a perf issue, on the other hand, we were expecting a perf gain without the polyfill, but given that late roslyn bug fix and the fact that it didn't make it to 17.4 preview 1 we will need to hold off on this.

So we don't expect a perf issue, it's clumsy but shippable if we must -- is that right?

Yes, this is shippable, but we would rather try again after RC1 ships for the same reasons we wanted to do it in the first place: we expect to see a significant perf increase, we want to build against newer compilers (which we have to do anyway), we want to use Roslyn APIs which are better tested than our polyfill approach.

@carlossanlop
Copy link
Member

Approved, signed off, CI failure is unrelated (System.Net.Http.Functional.Tests, known).
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit ba86f66 into dotnet:release/7.0 Sep 13, 2022
@joperezr joperezr deleted the RevertGeneratorsRoslyn branch September 13, 2022 14:35
@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants