-
Notifications
You must be signed in to change notification settings - Fork 193
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
Serialize Razor code document #8832
base: main
Are you sure you want to change the base?
Conversation
FWIW, it worries me a bit that we're going further down the rabbit hole of using JSON.NET and lots of JsonConverters. We're actively trying to get away from that in the tooling layers for performance and lower allocation. Is there any reason to use JSON.NET here? Or, could this use System.Text.Json or MessagePack? Or, is the intent to just get things working now and rework it later? |
I wrote this code some time ago and did not know this back then.
So I guess this is the current intent.
Wanted to use the existing TagHelperDescriptor converters. Although now there are more converters in the tooling layer, when I started I'm not sure if they weren't there or I didn't know about them, anyway this PR basically uses some other (legacy) converters from compiler layer. Specifically these ones: Lines 17 to 21 in ab893b6
|
FWIW, there is no longer a The goal for serialization in the tooling layer is to have an abstraction that we can use (in the near future) to dedupe common object instances and strings in the data, and (again later) switch to a more efficient serialization system. I hope we can just build on this work eventually, though I totally get wanting to just get things working now. This an area that I've actively been working on due to performance analysis that have pointed to our serialization. |
e5c9ca0
to
fcdbecf
Compare
eng/SourceBuildPrebuiltBaseline.xml
Outdated
@@ -5,7 +5,9 @@ | |||
<IgnorePatterns> | |||
<UsagePattern IdentityGlob="Microsoft.SourceBuild.Intermediate.*/*" /> | |||
|
|||
<!-- | |||
<Usage Id="Newtonsoft.Json" Version="13.0.3" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dotnet/source-build-internal how do I fix "new prebuilts detected" for Newtonsoft.Json? Is this fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking @jjonescz. Source build must build all dependent binaries from source. External dependencies such as Newtonsoft.Json are contained in the https://github.com/dotnet/source-build-externals repo. To fix this prebuilt, you simply need to add the appropriate version.details.xml dependency to this repo and add a darc subscription to keep it up to date.
<Dependency Name="Microsoft.SourceBuild.Intermediate.source-build-externals" Version="8.0.0-alpha.1.23312.1">
<Uri>https://github.com/dotnet/source-build-externals</Uri>
<Sha>50600f7fcd6e56a496233a859ed7d4d90c173b0d</Sha>
<SourceBuild RepoName="source-build-externals" ManagedOnly="true" />
</Dependency>
When razor CI runs the source-build leg, this package will be pulled in by the source-build infrastructure from source-build-externals and therefore will not be considered a prebuilt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I originally tried, but it doesn't get rid of the error. I redid the change, so you can see - it fails currently in CI. Can the reason be that I'm using Newtonsoft.Json version 13.0.3 but there's 13.0.1 in source-build-externals (I think)? But using 13.0.1 explicitly would get NuGet complaining about package downgrade (since there's Newtonsoft.Json 13.0.3 flowing into other projects from Roslyn) - although I guess I could use 13.0.1 for this source-built project and keep 13.0.3 for others if that will solve the problem...
fcdbecf
to
561f808
Compare
Resolves #8643.
cc @chsienki