up go 1.22.3 #4861

pfi79 · 2024-05-16T12:32:57Z

The following changes have been made:

added new possible error based on platform and Go release
changed the work with encoding/json - https://go.dev/doc/go1.22#minor_library_changes.
Marshaling and encoding functionality now escapes '\b' and '\f' characters as \b and \f instead of \u0008 and \u000c.
The creators of go overmuch with the change of cover work. So far I have turned on the old flag of cover operation - GOEXPERIMENT=nocoverageredesign. I will open a question in the go community. Most likely in the next release of go it will be necessary to remove this flag - cmd/go: go test -cover fails when loading plugin built without coverage golang/go#67427.

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>

denyeart

Thank you for the 1.22 update, quite timely!

denyeart · 2024-05-16T13:21:56Z

.github/workflows/verify-build.yml

@@ -37,6 +37,8 @@ jobs:
    name: Unit Tests
    needs: basic-checks
    runs-on: ${{ github.repository == 'hyperledger/fabric' && 'fabric-ubuntu-20.04' || 'ubuntu-20.04' }}
+    env:
+      GOEXPERIMENT: nocoverageredesign


What exactly breaks if we don't add GOEXPERIMENT=nocoverageredesign ?

golang/go#67427

--- FAIL: TestEndorsementPlugin (3.03s) panic: Error opening plugin at path /var/folders/6v/kmxqr0yn61588pbz646cfdyc0000gq/T/TestEndorsementPlugin4080854438/001/endorsementplugin.so: plugin.Open("/var/folders/6v/kmxqr0yn61588pbz646cfdyc0000gq/T/TestEndorsementPlugin4080854438/001/endorsementplugin"): plugin was built with a different version of package github.com/hyperledger/fabric/core/handlers/endorsement/api [recovered] panic: Error opening plugin at path /var/folders/6v/kmxqr0yn61588pbz646cfdyc0000gq/T/TestEndorsementPlugin4080854438/001/endorsementplugin.so: plugin.Open("/var/folders/6v/kmxqr0yn61588pbz646cfdyc0000gq/T/TestEndorsementPlugin4080854438/001/endorsementplugin"): plugin was built with a different version of package github.com/hyperledger/fabric/core/handlers/endorsement/api

go test -v -cover github.com/hyperledger/fabric/core/handlers/endorsement/api github.com/hyperledger/fabric/core/handlers/library

from github action:

2024-05-15 16:03:16.284 UTC 0001 PANI [core.handlers] loadPlugin -> Error opening plugin at path /tmp/TestEndorsementPlugin3801377298/001/endorsementplugin.so: plugin.Open("/tmp/TestEndorsementPlugin3801377298/001/endorsementplugin"): plugin was built with a different version of package github.com/hyperledger/fabric/core/handlers/endorsement/api --- FAIL: TestEndorsementPlugin (2.23s) panic: Error opening plugin at path /tmp/TestEndorsementPlugin3801377298/001/endorsementplugin.so: plugin.Open("/tmp/TestEndorsementPlugin3801377298/001/endorsementplugin"): plugin was built with a different version of package github.com/hyperledger/fabric/core/handlers/endorsement/api [recovered] panic: Error opening plugin at path /tmp/TestEndorsementPlugin3801377298/001/endorsementplugin.so: plugin.Open("/tmp/TestEndorsementPlugin3801377298/001/endorsementplugin"): plugin was built with a different version of package github.com/hyperledger/fabric/core/handlers/endorsement/api goroutine 34 [running]: testing.tRunner.func1.2({0x19efbc0, 0xc00041a5a0}) /opt/hostedtoolcache/go/1.22.3/x64/src/testing/testing.go:1631 +0x3f7 testing.tRunner.func1() /opt/hostedtoolcache/go/1.22.3/x64/src/testing/testing.go:1634 +0x6b6 panic({0x19efbc0?, 0xc00041a5a0?}) /opt/hostedtoolcache/go/1.22.3/x64/src/runtime/panic.go:770 +0x132 go.uber.org/zap/zapcore.CheckWriteAction.OnWrite(0x2, 0xc000166820, {0x4a?, 0xc000264300?, 0x17b?}) /home/runner/work/fabric/fabric/vendor/go.uber.org/zap/zapcore/entry.go:196 +0x9f go.uber.org/zap/zapcore.(*CheckedEntry).Write(0xc000166820, {0x0, 0x0, 0x0}) /home/runner/work/fabric/fabric/vendor/go.uber.org/zap/zapcore/entry.go:262 +0x3ad go.uber.org/zap.(*SugaredLogger).log(0xc0002b8e70, 0x4, {0xc000276360, 0x117}, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}) /home/runner/work/fabric/fabric/vendor/go.uber.org/zap/sugar.go:316 +0x130 go.uber.org/zap.(*SugaredLogger).Panicf(...) /home/runner/work/fabric/fabric/vendor/go.uber.org/zap/sugar.go:202 github.com/hyperledger/fabric-lib-go/common/flogging.(*FabricLogger).Panicf(...) /home/runner/work/fabric/fabric/vendor/github.com/hyperledger/fabric-lib-go/common/flogging/zap.go:74 github.com/hyperledger/fabric/core/handlers/library.(*registry).loadPlugin(0xc000443e18, {0xc00003a840, 0x3d}, 0x2, {0xc000443e98, 0x1, 0x1}) /home/runner/work/fabric/fabric/core/handlers/library/plugin.go:30 +0x3a9 github.com/hyperledger/fabric/core/handlers/library.TestEndorsementPlugin(0xc00037d040) /home/runner/work/fabric/fabric/core/handlers/library/registry_plugin_test.go:100 +0x1e7 testing.tRunner(0xc00037d040, 0x1c[215](https://github.com/pfi79/fabric/actions/runs/9098906093/job/25010318274#step:5:216)88) /opt/hostedtoolcache/go/1.22.3/x64/src/testing/testing.go:1689 +0x21f created by testing.(*T).Run in goroutine 1 /opt/hostedtoolcache/go/1.22.3/x64/src/testing/testing.go:1742 +0x826 FAIL github.com/hyperledger/fabric/core/handlers/library 31.645s

Have you tried to resolve this test failure without the workaround? Or is that what you are asking in the go community? I wouldn't want to use a workaround which may go away in the next release. But maybe you are suggesting this is a temporary Go issue that will get resolved by next release?

Have you tried to resolve this test failure without the workaround? Or is that what you are asking in the go community? I wouldn't want to use a workaround which may go away in the next release. But maybe you are suggesting this is a temporary Go issue that will get resolved by next release?

Yes I have tried various ways to solve the problem, assuming the error is somewhere in our fabric. But so far my conclusion is that the error is in go.

I went through all the questions in the go community on this topic and realized the following:

they redesigned the cover tool.

There are a lot of comments on the new cover. The developers themselves recognize this and are waiting for fixes.

so they introduced the GOEXPERIMENT=nocoverageredesign flag.

What happens next:

some fixes to the new cover will be made. I don't know to what extent.

At the end GOEXPERIMENT=nocoverageredesign will be canceled.

I hope that by that time in new go releases our problem will be solved.
I highlighted with my question that their new cover created a problem for us. I will wait and hope that the developers will see and respond to my question. Maybe they will offer some other alternative solution.

C0rWin · 2024-05-16T18:03:57Z

The creators of go overmuch with the change of cover work. So far I have turned on the old flag of cover operation - GOEXPERIMENT=nocoverageredesign. I will open a question in the go community. Most likely in the next release of go it will be necessary to remove this flag.

I do not think this is should be a fix and need to have better understanding of the root cause for the unit tests in failure.

pfi79 · 2024-05-17T06:02:48Z

The creators of go overmuch with the change of cover work. So far I have turned on the old flag of cover operation - GOEXPERIMENT=nocoverageredesign. I will open a question in the go community. Most likely in the next release of go it will be necessary to remove this flag.

I do not think this is should be a fix and need to have better understanding of the root cause for the unit tests in failure.

All right. I got it. I'll find a better explanation for why they're like this.

pfi79 · 2024-05-17T14:26:10Z

@C0rWin @denyeart

I was able to simulate the error.
The error is definitely not in fabric, it's in go. At a certain stage the test has an extra file covervars.go. Because of this, the test and plugin have different versions of the same package.

I'll pass it all on to the go developers from here on out.

Here's my example of the error appearing.
https://github.com/pfi79/go-test-cover-error

denyeart · 2024-05-17T14:50:53Z

@C0rWin @denyeart

I was able to simulate the error. The error is definitely not in fabric, it's in go. At a certain stage the test has an extra file covervars.go. Because of this, the test and plugin have different versions of the same package.

I'll pass it all on to the go developers from here on out.

Here's my example of the error appearing. https://github.com/pfi79/go-test-cover-error

Ok, thanks for investigating and opening the dialog with the Go team. Please add a link to that conversation here. Let's keep this PR on hold for now, perhaps Go team will offer another solution. Alternatively, if they agree to work on a fix we could potentially use the GOEXPERIMENT=nocoverageredesign workaround until the fix is available.

pfi79 · 2024-05-17T15:25:16Z

@denyeart @C0rWin I got an answer.

The error is no longer reproduced in version 1.23.

There are 2 variants of action:

Wait for go version 1.23 and try it already.
Now accept pr with GOEXPERIMENT=nocoverageredesign. When go 1.23 appears, check and if successful, remove GOEXPERIMENT=nocoverageredesign.

pfi79 · 2024-05-17T15:58:10Z

I'll add more details

go developers started to change cover to allow to take into account those packages that do not have test files, but functions are called from them when running tests.

Further implementation problems:

when forming a test plan, when several packages are passed there, their compilation cache is common for all tests.
core/handlers/endorsement/api package as a separate package for testing is processed in a special way and some covervars.go file is added to its cache.
our test from core/handlers/library depends on the core/handlers/endorsement/api package and with the cover.go file compiles it into the body of the executable.
Next, the test sends a command to build a plugin. The plugin also depends on core/handlers/endorsement/api, but it is built independently of the test. Therefore, it does not know anything about the covervars.go file.
And we try to open the plugin, but the core/handlers/endorsement/api package is really different in the test and in the plugin

denyeart · 2024-05-17T16:05:30Z

Since it appears to be resolved in upcoming Go 1.23, I'd be willing to live with the workaround GOEXPERIMENT=nocoverageredesign for the duration of Go 1.22. Let's see if others agree...

C0rWin · 2024-05-19T09:39:32Z

Since it appears to be resolved in upcoming Go 1.23, I'd be willing to live with the workaround GOEXPERIMENT=nocoverageredesign for the duration of Go 1.22. Let's see if others agree...

Well, this is not really a production code matter, so I think it's fine. However, IMO, we can just skip 1.22 and wait for 1.23 since I do not think there is something urgent that might require an immediate update to 1.22.

denyeart · 2024-05-19T14:21:06Z

Since it appears to be resolved in upcoming Go 1.23, I'd be willing to live with the workaround GOEXPERIMENT=nocoverageredesign for the duration of Go 1.22. Let's see if others agree...

Well, this is not really a production code matter, so I think it's fine. However, IMO, we can just skip 1.22 and wait for 1.23 since I do not think there is something urgent that might require an immediate update to 1.22.

I do like to step through every Go release... if some problem shows up it is easier to pinpoint the issue by looking at that version's Go release notes. There will also be a growing list of CVEs on Go 1.21 so I'm motivated to get to 1.22 soon and backport for a v2.5.8 release.

Let's see if there are any other comments in the next day, otherwise I'll plan to approve and merge.

denyeart

Ok, no other comments, I'll go ahead and merge, thank you @pfi79 !

Bump Go to 1.22.3. See details in hyperledger#4861. Signed-off-by: David Enyeart <enyeart@us.ibm.com>

Bump Go to 1.22.3. See details in #4861. Signed-off-by: David Enyeart <enyeart@us.ibm.com>

pfi79 requested review from a team as code owners May 16, 2024 12:32

up go 1.22.3

c95a763

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>

pfi79 force-pushed the up-go-1.22 branch from f2594bd to c95a763 Compare May 16, 2024 12:40

denyeart reviewed May 16, 2024

View reviewed changes

denyeart approved these changes May 20, 2024

View reviewed changes

denyeart merged commit 88c4134 into hyperledger:main May 20, 2024
14 of 15 checks passed

pfi79 deleted the up-go-1.22 branch May 20, 2024 21:10

denyeart added a commit to denyeart/fabric that referenced this pull request May 21, 2024

Bump Go to 1.22.3 (backport release-2.5)

375291d

Bump Go to 1.22.3. See details in hyperledger#4861. Signed-off-by: David Enyeart <enyeart@us.ibm.com>

denyeart mentioned this pull request May 21, 2024

Bump Go to 1.22.3 (backport release-2.5) #4866

Merged

C0rWin pushed a commit that referenced this pull request May 21, 2024

Bump Go to 1.22.3 (backport release-2.5) (#4866)

6ea5057

Bump Go to 1.22.3. See details in #4861. Signed-off-by: David Enyeart <enyeart@us.ibm.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

up go 1.22.3 #4861

up go 1.22.3 #4861

pfi79 commented May 16, 2024 •

edited

denyeart left a comment

denyeart May 16, 2024

pfi79 May 16, 2024

pfi79 May 16, 2024

pfi79 May 16, 2024

denyeart May 16, 2024

pfi79 May 16, 2024

C0rWin commented May 16, 2024

pfi79 commented May 17, 2024

pfi79 commented May 17, 2024

denyeart commented May 17, 2024

pfi79 commented May 17, 2024

pfi79 commented May 17, 2024

denyeart commented May 17, 2024

C0rWin commented May 19, 2024

denyeart commented May 19, 2024

denyeart left a comment

up go 1.22.3 #4861

up go 1.22.3 #4861

Conversation

pfi79 commented May 16, 2024 • edited

denyeart left a comment

Choose a reason for hiding this comment

denyeart May 16, 2024

Choose a reason for hiding this comment

pfi79 May 16, 2024

Choose a reason for hiding this comment

pfi79 May 16, 2024

Choose a reason for hiding this comment

pfi79 May 16, 2024

Choose a reason for hiding this comment

denyeart May 16, 2024

Choose a reason for hiding this comment

pfi79 May 16, 2024

Choose a reason for hiding this comment

C0rWin commented May 16, 2024

pfi79 commented May 17, 2024

pfi79 commented May 17, 2024

denyeart commented May 17, 2024

pfi79 commented May 17, 2024

pfi79 commented May 17, 2024

denyeart commented May 17, 2024

C0rWin commented May 19, 2024

denyeart commented May 19, 2024

denyeart left a comment

Choose a reason for hiding this comment

pfi79 commented May 16, 2024 •

edited