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

Make JsonGenerator be an incremental generator #57088

Merged
merged 12 commits into from
Aug 27, 2021

Conversation

chsienki
Copy link
Contributor

@chsienki chsienki commented Aug 9, 2021

No description provided.

@ghost
Copy link

ghost commented Aug 9, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: chsienki
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

eng/Versions.props Outdated Show resolved Hide resolved
@chsienki chsienki changed the title Make JsonGenerator be an incremental generator [Example / NoMerge] Make JsonGenerator be an incremental generator Aug 9, 2021
@jkoritzinsky
Copy link
Member

It might also be interesting to look at moving the EventSource generator to be an incremental generator as that one is currently internal-only and doesn't ship outside the repo. As a result, that generator can move to the incremental APIs before a cross-targeting story for generators is finalized.

@eerhardt eerhardt marked this pull request as ready for review August 25, 2021 23:29
@eerhardt eerhardt requested a review from layomia as a code owner August 25, 2021 23:29
@eerhardt eerhardt changed the title [Example / NoMerge] Make JsonGenerator be an incremental generator Make JsonGenerator be an incremental generator Aug 25, 2021
@steveharter
Copy link
Member

Is #58068 going to be merged first? (some overlap)

@steveharter
Copy link
Member

I assume the CI failures in STJ.SourceGeneration.Unit.Tests (due to the commit here that enabled the src-gen tests to run) will be addressed in this PR.

@eerhardt
Copy link
Member

Is #58068 going to be merged first? (some overlap)

They are going to go in at roughly the same time. The overlap in Versions.props should merge cleanly since they are identical changes.

I assume the CI failures in STJ.SourceGeneration.Unit.Tests (due to the commit here that enabled the src-gen tests to run) will be addressed in this PR.

Yes, fixing. The issue is that CI runs a few legs in Spanish, and this test was checking for hard-coded English strings.

@@ -227,6 +227,9 @@
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Security.Cryptography.Primitives\tests\System.Security.Cryptography.Primitives.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Security.Cryptography.Xml\tests\System.Security.Cryptography.Xml.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Security.Cryptography.X509Certificates\tests\System.Security.Cryptography.X509Certificates.Tests.csproj" />

Copy link
Member

Choose a reason for hiding this comment

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

FWIW this change under the condition:

<ItemGroup Condition="'$(TargetOS)' == 'Browser' and '$(BuildAOTTestsOnHelix)' == 'true' and '$(RunDisabledWasmTests)' != 'true'">

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the comment.

There are different levels of "exclude a test for wasm". This ItemGroup excludes the test for all WASM. The next group down excludes the test for "AOT" and "EnableAggressiveTrimming" WASM legs.

Copy link
Member

Choose a reason for hiding this comment

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

Just adding a note since the raw github view here doesn't show the parent condition (since it is too far above). May save someone from opening the .cs file and manually finding that LOC.

@eerhardt eerhardt merged commit 529f0e1 into dotnet:main Aug 27, 2021
@eerhardt
Copy link
Member

/backport to release/6.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1174954271

@github-actions
Copy link
Contributor

@eerhardt backporting to release/6.0-rc1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Make JsonGenerator be an incremental generator
Using index info to reconstruct a base tree...
M	eng/Versions.props
M	src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
M	src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
M	src/libraries/System.Text.Json/gen/JsonSourceGenerator.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Text.Json/gen/JsonSourceGenerator.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/gen/JsonSourceGenerator.cs
Auto-merging src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
Auto-merging src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Auto-merging eng/Versions.props
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make JsonGenerator be an incremental generator
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

eerhardt added a commit to eerhardt/runtime that referenced this pull request Aug 27, 2021
* Make JsonGenerator be an incremental generator

* Improve incrementalism by doing less work when not applicable

* Change SourceGeneration.UnitTests to SourceGeneration.Unit.Tests so it is built and executed in CI

* Get unit tests running after IIncrementalGenerator migration

* Fix duplicate file name tests by working around dotnet/roslyn#54185.

* Fix unit tests now that they are running in CI against non-English languages.

* Fix System.Text.Json.SourceGeneration.Unit.Tests on WASM

* Disable STJ.SourceGeneration.Unit.Tests on Browser

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Anipik pushed a commit that referenced this pull request Aug 27, 2021
* Make JsonGenerator be an incremental generator

* Improve incrementalism by doing less work when not applicable

* Change SourceGeneration.UnitTests to SourceGeneration.Unit.Tests so it is built and executed in CI

* Get unit tests running after IIncrementalGenerator migration

* Fix duplicate file name tests by working around dotnet/roslyn#54185.

* Fix unit tests now that they are running in CI against non-English languages.

* Fix System.Text.Json.SourceGeneration.Unit.Tests on WASM

* Disable STJ.SourceGeneration.Unit.Tests on Browser

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants