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

govet: skip fieldalignment test on 32bit platforms #5463

Merged
merged 1 commit into from
Feb 22, 2025

Conversation

realsn0w
Copy link
Contributor

@realsn0w realsn0w commented Feb 22, 2025

the govet fieldalignment test currently fails on 32bit targets because it assumes uintptr always has a size of 8 bytes.
this pr (edited after some discussion) adds build tags to skip it on platforms where that is not the case.

Copy link

boring-cyborg bot commented Feb 22, 2025

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Feb 22, 2025

CLA assistant check
All committers have signed the CLA.

@ldez ldez self-requested a review February 22, 2025 18:23
@ldez
Copy link
Member

ldez commented Feb 22, 2025

Hello, in which context do you need to run our tests inside a 32-bit arch?

@ldez ldez added the feedback required Requires additional feedback label Feb 22, 2025
@realsn0w
Copy link
Contributor Author

ran into this while working on the golangci-lint void package.
ci there runs a check stage after build for non-cross targets.
here is an example of a build failure caused by this.

also just in general, i feel like it would be nice if the tests work for all of the platforms you currently support / provide official binaries for (which currently includes armv6/7 and i386).

@ldez ldez added area: tests Continuous integration, tests and other checks and removed feedback required Requires additional feedback labels Feb 22, 2025
@ldez ldez added this to the unreleased milestone Feb 22, 2025
@ldez
Copy link
Member

ldez commented Feb 22, 2025

We will not be able to maintain those tests because our CI doesn't use 32-bit arch (we will not add it inside our CI) and globally 32-bit arches are dead but 🤷

I don't understand why distribution packagers want to recompile everything and run tests: this is a Go program (statically linked) with no usage of CGO, not a C program dynamically linked.
A re-compilation has no real advantages in this context, and running tests is a bit of a waste of CPU cycles because we already run them, but I don't want to enter into a debate here.

I will just ask to remove 32-bit tests because they will never be run inside our CI.
We will keep the build tags.

@ldez ldez added the feedback required Requires additional feedback label Feb 22, 2025
@realsn0w
Copy link
Contributor Author

alright, that sounds like a fair compromise.
thanks for the quick replies 🙂

@realsn0w realsn0w force-pushed the fixup/govet-fieldalignment-32bit branch from 1e5e980 to 263829a Compare February 22, 2025 20:37
@realsn0w realsn0w changed the title govet: add 32bit variant of fieldalignment test govet: skip fieldalignment test on 32bit platforms Feb 22, 2025
@ldez ldez removed the feedback required Requires additional feedback label Feb 22, 2025
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit 53d58e8 into golangci:master Feb 22, 2025
18 checks passed
@realsn0w realsn0w deleted the fixup/govet-fieldalignment-32bit branch February 23, 2025 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tests Continuous integration, tests and other checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants