Switch Razor to using clasp as a source package #10139
Conversation
dibarbet
force-pushed
the
clasp_source_package
branch
from
009138d
to
5164361
Compare
| <MicrosoftSourceBuildIntermediateroslynPackageVersion>4.10.0-3.24169.7</MicrosoftSourceBuildIntermediateroslynPackageVersion> | ||
| <MicrosoftVisualStudioLanguageServicesPackageVersion>4.10.0-3.24169.7</MicrosoftVisualStudioLanguageServicesPackageVersion> | ||
| <MicrosoftNetCompilersToolsetPackageVersion>4.10.0-3.24170.4</MicrosoftNetCompilersToolsetPackageVersion> | ||
| <MicrosoftCommonLanguageServerProtocolFrameworkPackageVersion>4.10.0-3.24170.4</MicrosoftCommonLanguageServerProtocolFrameworkPackageVersion> |
There was a problem hiding this comment.
Firstly, assume I know nothing about our infra, but do we want to change things so that this is no longer automatically updated by our darc subscriptions? Seems like if we don't do that, you're just as likely to break us if you make changes to things.
Having said that, I've no idea if these ever actually automatically update, as I've just been doing it manually when I need to :)
There was a problem hiding this comment.
I have no idea if roslyn ide packages can be upgraded by darc subscriptions. But either way you don't have any subscriptions in Razor from Roslyn for any of your normal branches, so I don't know why you have versions in version.details.xml
From darc:
PS C:\Users\dabarbet\source\repos\razor> darc get-subscriptions --source-repo roslyn --target-repo razor
https://github.com/dotnet/roslyn (General Testing) ==> 'https://github.com/dotnet/razor' ('demo/lexer')
- Id: a20bdf0a-4619-4e57-3a2c-08dc325cea30
- Update Frequency: None
- Enabled: True
- Batchable: False
- PR Failure Notification tags:
- Merge Policies: []
PS C:\Users\dabarbet\source\repos\razor>
There was a problem hiding this comment.
I updated the version details file as well since CI was complaining. I assume there must be some other reason for having the versions listed there too, but there aren't any subscriptions using it as far as I can tell.
There was a problem hiding this comment.
Yeah, I have no idea either, I just always update both files 🤷♂️
dibarbet
force-pushed
the
clasp_source_package
branch
from
5164361
to
f042b6f
Compare
dibarbet
force-pushed
the
clasp_source_package
branch
from
4556f8f
to
e1ce0f6
Compare
eng/common/tools.sh
Outdated
| @@ -477,6 +477,11 @@ function MSBuild { | |||
| args+=( "-logger:$selectedPath" ) | |||
| fi | |||
|
|
|||
| # Workaround for a bug where .editorconfig files in nuget packages are not respected if the NUGET_PACKAGES path does not end with a slash | |||
There was a problem hiding this comment.
This is insane I never know how you smart people find these wacky edge cases. Truly impressed 😲
There was a problem hiding this comment.
Agreed 💯! @dibarbet routinely blows my mind. 😄
There was a problem hiding this comment.
I can figure this out but can't figure out how to call a shell script in CI 😢
dibarbet
force-pushed
the
clasp_source_package
branch
2 times, most recently
from
415a1fb
to
ed867b2
Compare
dibarbet
force-pushed
the
clasp_source_package
branch
6 times, most recently
from
6f32eea
to
1a4c164
Compare
dibarbet
force-pushed
the
clasp_source_package
branch
2 times, most recently
from
ca65924
to
1b322ac
Compare
dibarbet
force-pushed
the
clasp_source_package
branch
from
1b322ac
to
0317025
Compare
|
@DustinCampbell @davidwengier @ryzngard would you mind taking a second look here? I had to do a couple messy workarounds |
| /// Wraps the razor logger (from the clasp source package) into the binary clasp logger that webtools uses. | ||
| /// </summary> | ||
| /// <param name="logger"></param> | ||
| private class LegacyClaspILspLogger(ILspLogger logger) : LegacyClasp.Microsoft.CommonLanguageServerProtocol.Framework.ILspLogger |
There was a problem hiding this comment.
I'm guessing that this is necessary until Web Tools is updated and then we pull in new Web Tools assemblies for tests? If so, could you please be sure to file an issue for us so we remember to get back to this?
There was a problem hiding this comment.
Yeah we can remove the code that ships the legacy clasp package. I filed #10160 for that.
However I expect this particular bit of code will actually need to change to a reflection instantiation of the ILspLogger from the webtools package (since the razor source package type isn't the same type as the webtools source package type). Its unfortunate that the clasp type is exposed across boundaries here - maybe there is a possibility on the webtools side to retrieve the logger in another way (that Razor wouldn't need to know about).
There was a problem hiding this comment.
Yeah, that makes sense. Of course, we use so much reflection for the WebTools code here that's really just business as usual. 😄
Summary of the changes
Now that dotnet/roslyn#72237 has merged on the Roslyn side, we need to update Razor to consume the source package version. This will allow us to delete the nuget package version
Fixes: