Skip to content

use experimental go json v2 library #35392

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 35 commits into from
Sep 28, 2025
Merged

use experimental go json v2 library #35392

merged 35 commits into from
Sep 28, 2025

Conversation

junoberryferry
Copy link
Contributor

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 31, 2025
techknowlogick
techknowlogick reviewed Aug 31, 2025
View reviewed changes
techknowlogick and others added 2 commits September 1, 2025 19:35
@techknowlogick
Merge branch 'main' into jsonv2
cb16b33
@techknowlogick
Update go.mod
abc734c 
Signed-off-by: techknowlogick <matti@mdranta.net>
techknowlogick
techknowlogick reviewed Sep 2, 2025
View reviewed changes
@techknowlogick
Update jsonv2.go
f9c8805 
Signed-off-by: techknowlogick <matti@mdranta.net>
techknowlogick
techknowlogick reviewed Sep 2, 2025
View reviewed changes
@techknowlogick
Update jsonv2.go
9d83440 
Signed-off-by: techknowlogick <matti@mdranta.net>
techknowlogick
techknowlogick reviewed Sep 2, 2025
View reviewed changes
@techknowlogick
Update jsonv2.go
31492c1 
Signed-off-by: techknowlogick <matti@mdranta.net>
techknowlogick
techknowlogick approved these changes Sep 2, 2025
View reviewed changes
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 2, 2025
@techknowlogick
Update jsonv2.go
7b0832c 
Signed-off-by: techknowlogick <matti@mdranta.net>
techknowlogick
techknowlogick approved these changes Sep 2, 2025
View reviewed changes
techknowlogick and others added 4 commits September 2, 2025 19:57
@techknowlogick
Update jsonv2_fallback.go
fe51938 
Signed-off-by: techknowlogick <matti@mdranta.net>
@techknowlogick
Update jsonv2_fallback.go
29d1770 
Signed-off-by: techknowlogick <matti@mdranta.net>
@techknowlogick
Update jsonv2_fallback.go
3c3f77f 
Signed-off-by: techknowlogick <matti@mdranta.net>
@techknowlogick
Merge branch 'main' into jsonv2
ce4107d
@techknowlogick
Copy link
Member

@junoberryferry I'm sorry for all the notifications. I've just resolved the linting errors on my phone. Thanks for this PR!

techknowlogick
techknowlogick approved these changes Sep 3, 2025
View reviewed changes
@techknowlogick
Update jsonv2_fallback.go
b8fb94d 
Signed-off-by: techknowlogick <matti@mdranta.net>
@lunny
Copy link
Member

lunny commented Sep 3, 2025

Since it's not enabled by default. GOEXPERIMENT=jsonv2 is needed. I don't think this should be merged into v1.25

techknowlogick
techknowlogick approved these changes Sep 3, 2025
View reviewed changes
@techknowlogick
Update jsonv2_fallback.go
c748ca8 
Signed-off-by: techknowlogick <matti@mdranta.net>
@wxiaoguang wxiaoguang force-pushed the jsonv2 branch 2 times, most recently from bcf9659 to 706cb44 Compare September 26, 2025 18:49
@wxiaoguang wxiaoguang force-pushed the jsonv2 branch 3 times, most recently from e63d111 to 47cc820 Compare September 26, 2025 19:36
@wxiaoguang wxiaoguang marked this pull request as ready for review September 26, 2025 20:04
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 26, 2025
@wxiaoguang wxiaoguang added this to the 1.26.0 milestone Sep 26, 2025
@wxiaoguang wxiaoguang force-pushed the jsonv2 branch 3 times, most recently from 46fa69e to d810180 Compare September 27, 2025 05:01
@wxiaoguang wxiaoguang removed their assignment Sep 27, 2025
@wxiaoguang
Copy link
Contributor

Anything else to change?

@wxiaoguang wxiaoguang enabled auto-merge (squash) September 28, 2025 07:37
@wxiaoguang wxiaoguang merged commit 151ef80 into go-gitea:main Sep 28, 2025
26 checks passed
silverwind
silverwind reviewed Sep 29, 2025
View reviewed changes
Makefile

# By default use go's 1.25 experimental json v2 library when building
# TODO: remove when no longer experimental
export GOEXPERIMENT ?= jsonv2
Copy link
Member

Choose a reason for hiding this comment

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

fyi we do not use submake, so this export is useless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why it is "useless"? Will go ... inherit non-exported variables?

Copy link
Member

@silverwind silverwind Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, all commands will see the variables like FOO ?= bar, export in a Makefile is only for sub-make:

https://www.gnu.org/software/make/manual/html_node/Variables_002fRecursion.html#Communicating-Variables-to-a-Sub_002dmake

Copy link
Contributor

@wxiaoguang wxiaoguang Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure or have you tested? I don't think the the GNU toolchain can have such a counterintuitive, global-polluting and dirty design.

image

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you are right. My test was incorrect. I have always used the explicit form VAR=$(VAR) cmd so far, which explicitly passes variables. I think it's the most clean way, especially because the Makefile handles a lot of other commands that don't accept GOEXPERIMENT.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that "export" is not the best approach, while it just works, so I just kept the original change from techknowlogick's 94d537c

rossigee pushed a commit to rossigee/gitea that referenced this pull request Oct 2, 2025
@junoberryferry @techknowlogick
@wxiaoguang @rossigee
use experimental go json v2 library (go-gitea#35392)
79077c6 
details: https://pkg.go.dev/encoding/json/v2

---------

Co-authored-by: techknowlogick <matti@mdranta.net>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
rossigee added a commit to rossigee/gitea that referenced this pull request Oct 4, 2025
@rossigee @techknowlogick @wxiaoguang
use experimental go json v2 library (go-gitea#35392)
2fc8c67 
details: https://pkg.go.dev/encoding/json/v2

---------

Co-authored-by: techknowlogick <matti@mdranta.net>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
rossigee pushed a commit to rossigee/gitea that referenced this pull request Oct 4, 2025
@junoberryferry @techknowlogick
@wxiaoguang @rossigee
use experimental go json v2 library (go-gitea#35392)
8f74cfb 
details: https://pkg.go.dev/encoding/json/v2

---------

Co-authored-by: techknowlogick <matti@mdranta.net>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
rossigee pushed a commit to rossigee/gitea that referenced this pull request Oct 4, 2025
@junoberryferry @techknowlogick
@wxiaoguang @rossigee
use experimental go json v2 library (go-gitea#35392)
aefa378 
details: https://pkg.go.dev/encoding/json/v2

---------

Co-authored-by: techknowlogick <matti@mdranta.net>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 5, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
06198a4 
* giteaofficial/main:
  fix: auto-expand and auto-scroll for actions logs (go-gitea#35570) (go-gitea#35583)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  Fix creating pull request failure when the target branch name is the same as some tag (go-gitea#35552)
  Use bundled version of spectral (go-gitea#35573)
  Add rebase push display wrong comments bug (go-gitea#35560)
  Address some CodeQL security concerns (go-gitea#35572)
  fix(webhook): prevent tag events from bypassing branch filters targets go-gitea#35449 (go-gitea#35567)
  Added button to copy file name in PR files (go-gitea#35509)
  Update JS and PY deps (go-gitea#35565)
  Enable a few more tsconfig options (go-gitea#35553)
  Bump github.com/wneessen/go-mail from 0.6.2 to 0.7.1 (go-gitea#35557)
  add more routes to the "expensive" list (go-gitea#35547)
  Drop json-iterator dependency (go-gitea#35544)
  Add proper error message if session provider can not be created (go-gitea#35520)
  use experimental go json v2 library (go-gitea#35392)
  Use global lock instead of status pool for cron lock (go-gitea#35507)
  Move some functions to gitrepo package (go-gitea#35503)
  Move GetDiverging functions to gitrepo (go-gitea#35524)
  [skip ci] Updated translations via Crowdin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silverwind silverwind silverwind left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/dependencies modifies/go Pull requests that update Go code modifies/internal
Projects
None yet
Milestone
1.26.0
Development

Successfully merging this pull request may close these issues.
6 participants
@junoberryferry @techknowlogick @lunny @wxiaoguang @silverwind @GiteaBot