-
Notifications
You must be signed in to change notification settings - Fork 610
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
[upstream_utils] Update to json 3.11.2 #5680
Conversation
I've got tests incoming, not managed by upstream utils because what I had to do is gross and still requires hand edits if you want them |
Some tests is better than no tests. I think we had to heavily modify them from upstream because they use Catch2 instead of GoogleTest. |
The windows debug CI is having trouble building the tests. I don't know if the debug symbols are too big or what. I can probably just add a subset of the test suite for the moment, or if you want to land this shortly I can do the tests in a separate PR. Its nice having the build cache once it gets pushed to main. |
The CI runner is running out of disk space. https://learn.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/compiler-error-c2471?view=msvc-170 |
Unfortunately this causes some real growth in artifact sizes etc, and will almost certainly break the installer size in addition to CI (as Linux is currently at 1.93G and the limit is 2G). I suspect this is due to all the duplicate templated codegen into object and debug info, and increased size in debug info due to the json symbols being significantly larger. Testing on windows ("was" is latest main, "is" is this PR): growth in all the artifact sizes (compare https://github.com/wpilibsuite/allwpilib/actions/runs/6279775375 with https://github.com/wpilibsuite/allwpilib/actions/runs/6281803893) |
IIRC the biggest headache going from the raw upstream to the detemplatized, split source and header version was the fact that I can take a whirl at getting that going with a straight jump from status quo to 3.11.2, but I might leave the automated process out of the PR, if that is acceptable. Upgrade now, automate later. |
I think using |
What is an tolerable increase in size? I've played around with separating implementation out of the header when possible, detemplatizing classes, etc, and I can get the size down from what the links Peter gave, but it is still bigger non-trivially bigger than the current implementation. I think something to consider when jumping from I see a beta tag was created. Based on that I can see a full version update dying on the vine like it did last year and I don't want it to completely fall on the floor again. Perhaps we could get |
We have some other changes and additions blocked on artifact size, so we're going to have to solve the artifact size problem properly for 2024 anyway. I wouldn't work too hard to shrink things in the JSON library beyond what you've already done (detemplating is nice for other reasons like having readable symbols in compiler errors). We'll likely host everything on Artifactory to avoid the 2 GB limit. For the record, I'd also like to see this merged for 2024 rather than languishing for another year. |
Less intrusive version of #5622 as it contains overloads for the llvm streams.