-
Notifications
You must be signed in to change notification settings - Fork 841
Fixes ARM build with const overflow. #1822
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
Conversation
see grpc/grpc-go#1471 Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
|
This test is now a no-op on a 32-bit platform: it can never be true. I’m not immediately convinced that this is the right way to go. |
This is fine, because you're unlikely to have more than 4gb slice in memory on 32-bit platform, so this check will never fail. It is there only to make sure that we don't overflow 32-bit serialized value few lines below. |
tomwilkie
left a comment
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.
I'm inclined to agree with Peter on this one.
gouthamve
left a comment
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.
tbh, I am not super sure why the compilation fails and what the compiler is doing. But adding this fixes the compilation and the change doesn't doesn't seem wrong.
➜ cortex git:(release-0.4) ✗ GOARCH=386 go build ./cmd/cortex
go: downloading google.golang.org/grpc v1.25.1
go: extracting google.golang.org/grpc v1.25.1
go: finding google.golang.org/grpc v1.25.1
# github.com/cortexproject/cortex/pkg/ring/kv/memberlist
pkg/ring/kv/memberlist/memberlist_client.go:619:21: constant 4294967295 overflows int
# github.com/thanos-io/thanos/pkg/store/cache
../../../../pkg/mod/github.com/thanos-io/thanos@v0.7.0/pkg/store/cache/cache.go:166:22: constant 9223372036854775807 overflows int
But Thanos is broken as well and needs a PR upstream.
|
I'm merging this as it's a Golang quirk. |
Fix changes left side of comparison to |
Since #1727 ARM build fails with an error:
see grpc/grpc-go#1471, it seems that this is cause by missing int conversion with MaxUint32.
I also run all tests
GOARCH=386 make testall good now.