-
Notifications
You must be signed in to change notification settings - Fork 391
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
feat(gnovm/pkg/gnolang): add build constraints to fail on 32-bit architectures #3643
base: master
Are you sure you want to change the base?
feat(gnovm/pkg/gnolang): add build constraints to fail on 32-bit architectures #3643
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Sounds reasonable. As the gno.land validator set will initially be limited, this probably isn't a pressing action. |
…itectures This change adds build constraints so as to panic if built for 32-bit architectures as it is a project wide decision not to support them. This allows the mainnet launch without sweating trying to make a bunch of runtime changes for the gnovm. Updates gnolang#3288
b411964
to
e746a78
Compare
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.
LGTM
remove: review/triage-pending
flag
…2-bit-architectures
…2-bit-architectures
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.
The idea looks good to me, but please check the comments for some improvements.
@@ -116,3 +116,6 @@ repository offers more resources to dig into. We are eager to see your first PR! | |||
* [![Go Reference](https://pkg.go.dev/badge/hey/google)](https://gnolang.github.io/gno/github.com/gnolang/gno.html) \ | |||
(pkg.go.dev will not show our repository as it has a license it doesn't recognise) | |||
</details> | |||
|
|||
## Declarations | |||
* Gno is only available for 64-bit architectures! |
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.
@leohhhn maybe you have a better idea of the right place(s) to add this info?
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.
definitely shouldn't go in README; maybe in pkg/gnolang/ readme.
Also we should update the goreleaser configuration to avoid 32bit targets.
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.
suggestion:
package main
import "strconv"
func _() {
var x [1]struct{}
_ = x[strconv.IntSize-64]
}
This is a trick used also by stringer
. It has the advantage that if the value of strconv.IntSize != 64
, then compilation fails entirely (rather than at runtime, as the code in the PR does).
Aside from the code above, you should add some comment to explain the compile error should someone come up with it.
Also the file shouldn't be named _386
to avoid it only building on 386. The code above should be added without any build constraints.
This change adds build constraints so as to panic if built for 32-bit architectures as it is a project wide decision not to support them.
This allows the mainnet launch without sweating trying to make a bunch of runtime changes for the gnovm.
Updates #3288