-
Notifications
You must be signed in to change notification settings - Fork 190
update to go1.25.1 #387
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
update to go1.25.1 #387
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Nope, hack didn't make a difference; I'll drop that commit;
|
So for some reason it could be that the size of what's stored changed, and that we now hit the limits of the maximum. Previously, we hit this for long passwords / tokens, but now it gives the same problem for tests in this repo;
This is effectively where things go wrong, but not sure if the size constraint is just for the blob ( docker-credential-helpers/wincred/wincred.go Lines 18 to 28 in d4602cd
|
wincred/wincred.go
Outdated
out, err := json.Marshal(g) | ||
if err != nil { | ||
fmt.Println("Error:", err) | ||
} else { | ||
fmt.Println("GenericCredential:", string(out)) | ||
} |
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.
Just for debugging; this is what's shown;
=== RUN TestWinCredHelper
GenericCredential: {"TargetName":"https://foobar.docker.io:2376/v1","Comment":"","LastWritten":"0001-01-01T00:00:00Z","CredentialBlob":"Zm9vYmFyYmF6","Attributes":[{"Keyword":"label","Value":"RG9ja2VyIENyZWRlbnRpYWxz"}],"TargetAlias":"","UserName":"foobar","Persist":2}
wincred_test.go:52: The stub received bad data.
Could it be something else, e.g. the LastWritten
date not being accepted or something? (because it's a zero date)?
I opened a PR against danieljoos/wincred to see if CI would fail there as well, but .. it looks like tests are passing there;
But I did notice that those set the This one's also interesting, because it doesn't fail with the same error, but DOES return a "The parameter is incorrect." error; docker-credential-helpers/wincred/wincred_test.go Lines 45 to 53 in d4602cd
Here's what we try to write; {"TargetName":"https://foobar.docker.io:2376/v1","Comment":"","LastWritten":"0001-01-01T00:00:00Z","CredentialBlob":"Zm9vYmFyYmF6","Attributes":[{"Keyword":"label","Value":"RG9ja2VyIENyZWRlbnRpYWxz"}],"TargetAlias":"","UserName":"foobar","Persist":2} And the {"Flags":0,"Type":1,"TargetName":104,"Comment":0,"LastWritten":{"LowDateTime":3201347093,"HighDateTime":11290211},"CredentialBlobSize":9,"CredentialBlob":824633762576,"Persist":2,"AttributeCount":1,"Attributes":824633908264,"TargetAlias":0,"UserName":102} Another one about "Parameter is incorrect";
|
Oh! And I just realise that ALL tests now produce a |
7e0140e
to
80a855b
Compare
Makefile
Outdated
test: | ||
mkdir -p $(COVERAGEDIR) | ||
go test -short -v -coverprofile=$(COVERAGEDIR)/coverage.txt -covermode=atomic ./... | ||
go test -gcflags=all=-d=variablemakehash=n -short -v -coverprofile=$(COVERAGEDIR)/coverage.txt -covermode=atomic ./... |
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.
OK; looks like I may have found some clues; it's go1.25 exposing bugs on how unsafe.Pointers are used; https://go.dev/doc/go1.25#faster-slices
Faster slices
The compiler can now allocate the backing store for slices on the stack in more situations, which improves performance. This change has the potential to amplify the effects of incorrect unsafe.Pointer usage, see for example issue 73199. In order to track down these problems, the bisect tool can be used to find the allocation causing trouble using the
-compile=variablemake
flag. All such new stack allocations can also be turned off using-gcflags=all=-d=variablemakehash=n
.
85d724f
to
b4688d2
Compare
e61e1e5
to
071737d
Compare
Signed-off-by: Sebastiaan van Stijn <[email protected]>
And .. the last one @crazy-max |
Uh oh!
There was an error while loading. Please reload this page.