Skip to content

Fix compilation error and test on intel 32-bit#234

Merged
zenhack merged 3 commits into
capnproto:mainfrom
Matherunner:fix-32bit
May 4, 2022
Merged

Fix compilation error and test on intel 32-bit#234
zenhack merged 3 commits into
capnproto:mainfrom
Matherunner:fix-32bit

Conversation

@Matherunner

Copy link
Copy Markdown
Contributor

This PR fixes the following.

Set

export GOARCH=386

Then run go test ./..., it will fail:

./message.go:790:14: cannot convert maxSegmentSize (constant 4294967288 of type Size) to type int

After fixing the compilation error, this test will fail:

--- FAIL: TestNextAlloc (0.00s)
    message_test.go:909: max size: nextAlloc(0, 9223372036854775800, 4294967288) = _, capnp: alloc 4294967288 bytes: too large; want >=4294967288, <nil>

Anecdotally, the fixed version on a 32-bit Windows application runs fine when encoding, decoding, and doing basic RPC.

@lthibault lthibault requested a review from zenhack May 3, 2022 19:52
zenhack
zenhack previously approved these changes May 4, 2022

@zenhack zenhack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Bonus points if you want to add 386 to our CI, so it catches word size problems in the future.

Go test -race doesn't work on linux/386.
@Matherunner

Copy link
Copy Markdown
Contributor Author

LGTM. Bonus points if you want to add 386 to our CI, so it catches word size problems in the future.

Good idea. Added 386 to the matrix and tested on a PR on my fork.

@lthibault

Copy link
Copy Markdown
Collaborator

@zenhack CI seems to be failing on the race condition we discovered a few days ago. I'm OK to merge this, since it's definitely not causing any new issues.

@lthibault

Copy link
Copy Markdown
Collaborator

P.S. @Matherunner : thank you very much for this fix! 🙂

@zenhack

zenhack commented May 4, 2022

Copy link
Copy Markdown
Contributor

Yeah, I agree; that's an existing failure and this doesn't touch anything but the tests so it's pretty low risk. Merging. Thanks @Matherunner!

@zenhack zenhack merged commit 7329fad into capnproto:main May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants