Skip to content
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

Fix building on 32-bit systems #1127

Closed
wants to merge 4 commits into from

Conversation

ItalyPaleAle
Copy link
Contributor

Builds on 32-bit systems are failing:

 /go/pkg/mod/github.com/lestrrat-go/jwx/v2@v2.0.21/jwt/openid/birthdate.go:69:7: math.MaxInt64 (untyped int constant 9223372036854775807) overflows int

This changes how 32-bit systems are detected in a way that doesn't cause the error. Although this does remove 16-bit systems from the case, the Go compiler does not support any 16-bit system at present (and it's very unlikely it ever will).

ItalyPaleAle added a commit to ItalyPaleAle/traefik-forward-auth that referenced this pull request May 11, 2024
Copy link

codecov bot commented May 12, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 75.10%. Comparing base (0a15b4d) to head (5a3d7f0).
Report is 73 commits behind head on develop/v2.

Files Patch % Lines
jwt/openid/birthdate.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           develop/v2    #1127      +/-   ##
==============================================
+ Coverage       72.00%   75.10%   +3.09%     
==============================================
  Files              93       95       +2     
  Lines           13795    11941    -1854     
==============================================
- Hits             9933     8968     -965     
+ Misses           3044     2157     -887     
+ Partials          818      816       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ItalyPaleAle
Copy link
Contributor Author

I'm not sure how to appease CodeQL at this point. It's a false positive but nothing I do seems to make it happy.

@lestrrat
Copy link
Collaborator

Thanks, but I'm a bit reluctant to merge something that I can't test.
Would you be able to provide a 32-bit environment smoke test, possibly by running a container or some such in GitHub Actions?

@ItalyPaleAle
Copy link
Contributor Author

Sadly GH Actions runs on amd64, and Docker runs 64-bit containers on 64-bit operating systems like our laptops.

Simplest thing you can do to test this, if you're on a Linux box with qemu-system-arm and qemu-user-binfmt installed (this will be very slow):

cd examples
GOOS=linux GOARCH=arm GOARM=7 go test

The go test command above will fail when cross-compiling for armv7 (32-bit), during the build stage, with this error:

# github.com/lestrrat-go/jwx/v2/jwt/openid
../jwt/openid/birthdate.go:69:7: math.MaxInt64 (untyped int constant 9223372036854775807) overflows int

If you check out this PR, you'll see the build passes and the tests (running using emulation) work.

@lestrrat
Copy link
Collaborator

@ItalyPaleAle Well, I wanted to make sure I get to test and fix the CodeQL problem.
Also, I'm not talking about you or this PR, but I've had my share of I-trusted-the-PR-without-much-scrutiny-and-it-bit-me-later types of problems, so I'm being cautious here :)

Anyways. I see how you verified the 32-bit compilation, so I've dug in and isolated the problem down to #1129 . This way there's no CodeQL shenanigans, and I think Go on 32-bit arch can compile it. Does this work for you?

@ItalyPaleAle
Copy link
Contributor Author

Did not take it personally at all, your request was very valid and reasonable :)

#1129 LGTM and I think will fix the problem while making CodeQL happy. I'll close this PR. Thanks!

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.

None yet

2 participants