Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Consolidate roslyn versions #8103

Merged
merged 2 commits into from
Feb 15, 2023
Merged

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jan 10, 2023

We had multiple sets of roslyn references, differing between the compiler and the tooling. This consolidates those versions and updates the compiler references to be inline with the tooling.

We had multiple sets of roslyn references, differing between the compiler and the tooling. This consolidates those versions and updates the compiler references to be inline with the tooling.
@333fred 333fred force-pushed the update-compiler-refs branch from 36cb038 to d649d7e Compare January 10, 2023 19:30
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

This makes sense BUT given that the compiler inserts into the SDK (and VS), and Tooling inserts into VS, is there any risk that the moving forward references in aid of VS, breaks the SDK?

@333fred 333fred marked this pull request as ready for review January 11, 2023 00:34
@333fred 333fred requested review from a team as code owners January 11, 2023 00:34
@333fred
Copy link
Member Author

333fred commented Jan 11, 2023

is there any risk that the moving forward references in aid of VS, breaks the SDK?

I'm honestly not certain. @chsienki, are you aware of concerns here?

@chsienki
Copy link
Contributor

is there any risk that the moving forward references in aid of VS, breaks the SDK?

I'm honestly not certain. @chsienki, are you aware of concerns here?

Hmm. Interesting question. Generally, we only care about pushing the compiler references beyond what's in VS to ensure that newer SDKs work with older VS's. There's definitely the possibility that we could move a ref forward for tooling purposes, that in turn breaks the source generator in the SDK in a non-back compatible way.

We've solved this in the past by having a specific 'SourceGenerator' version property that is moved in line with what we're allowed in the SDK. That probably makes sense to do here as well, effectively having two versions of Roslyn that we target, where one is always lower than the other

@333fred
Copy link
Member Author

333fred commented Jan 11, 2023

We've solved this in the past by having a specific 'SourceGenerator' version property that is moved in line with what we're allowed in the SDK. That probably makes sense to do here as well, effectively having two versions of Roslyn that we target, where one is always lower than the other

I'm not sure how this would work with the EA assembly for the host outputs api prototype. That will require the source generator to be on pre-release roslyn.

Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

Think we're good now, because we're loading the generator from the tooling, so it shouldn't be possible for it to affect an older VS. That being said it could technically be possible for VS to be ahead of the SDK in terms of Roslyn version, but in reality, I don't think that ever happens, so we're probably ok.

@333fred 333fred merged commit 9c4bd43 into dotnet:main Feb 15, 2023
@333fred 333fred deleted the update-compiler-refs branch February 15, 2023 21:06
333fred added a commit to 333fred/razor that referenced this pull request Feb 15, 2023
* upstream/main: (29 commits)
  Consolidate roslyn versions (dotnet#8103)
  Don't set always-auth
  Fix darc publishing (dotnet#8275)
  Update LSP protocol DLL version
  reducing line comment length
  Avoid killing the process when a login window is closed.
  Clean up serialization benchmarks
  Accepting applicationUrl with more than one url. "applicationUrl": "http://localhost:7104;https://localhost:7105"
  Removing more code related to blazorwasmdebuggingextension
  Add More Data For RemoteInvocationException (dotnet#8240)
  Trying to completely remove the BlazorWasmDebuggingExtension and getting URL information from launchSettings.json
  Cleanup
  Fix TextMate missing item
  Use ImmutableHashSet rather than HashSet in LegacySyntaxNodeExtensions
  Use more efficient stack for FlattenSpansInReverse
  Use an object pool rather than a thread-static
  Remove unneeded 'aggressive inlining' attributes
  Add lock to protect against multi-threaded access
  Rename fields to use _lazy prefix
  Clean up and further optimizations in LegacySyntaxNodeExtensions
  ...
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants