Skip to content

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.
@ghost
Copy link

ghost commented Sep 12, 2022

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
Contributor

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