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 32bit builds #157

Merged
merged 2 commits into from
Jul 24, 2017
Merged

*: fix 32bit builds #157

merged 2 commits into from
Jul 24, 2017

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Jul 22, 2017

80f787e ("*: replace syscall with unix") did not correctly replace
the usage of (*syscall.Stat_t).[AM]tim.Unix() in tests, which resulted
in 32-bit builds failing in OBS because we run those tests in a %check
section. We need to add CI testing to make sure that we don't miss stuff
like this in the future.

Fixes: 80f787e ("*: replace syscall with unix")
Signed-off-by: Aleksa Sarai asarai@suse.de

@cyphar cyphar force-pushed the travis-test-32bit branch 3 times, most recently from 7e0ca46 to ee719d9 Compare July 23, 2017 20:30
80f787e ("*: replace syscall with unix") did not correctly replace
the usage of (*syscall.Stat_t).[AM]tim.Unix() in tests, which resulted
in 32-bit builds failing in OBS because we run those tests in a %check
section. We need to add CI testing to make sure that we don't miss stuff
like this in the future.

Fixes: 80f787e ("*: replace syscall with unix")
Signed-off-by: Aleksa Sarai <asarai@suse.de>
@cyphar cyphar changed the title travis: always build 32bit *: fix 32bit builds Jul 23, 2017
@cyphar
Copy link
Member Author

cyphar commented Jul 23, 2017

Sorry I missed this in #141. I've added some checks to CI to avoid this happening in the future.

@cyphar cyphar force-pushed the travis-test-32bit branch 3 times, most recently from 9e7410b to 53ecd84 Compare July 24, 2017 00:44
This also adds a local-validate-build target which validates that the
project can be built (including the tests), which is then run as part of
our CI.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
@cyphar
Copy link
Member Author

cyphar commented Jul 24, 2017

LGTM.

@cyphar cyphar merged commit d043ade into opencontainers:master Jul 24, 2017
cyphar added a commit that referenced this pull request Jul 24, 2017
@cyphar cyphar deleted the travis-test-32bit branch July 24, 2017 02:12
@vrothberg
Copy link
Contributor

@cyphar I am surprised this fixes the mentioned issue as Timespec.Unix() does effectively the same thing plus a cast (which is redundant IMHO as both members are of type int64 [1]). I feel that I miss some important piece. Would you explain why using Unix() fixes the issue?

[1] https://godoc.org/golang.org/x/sys/unix#Timespec

@cyphar
Copy link
Member Author

cyphar commented Jul 24, 2017

@vrothberg On 32-bit systems, Timespec.Sec and Timespec.Nsec are int32 which will cause compilation errors when used directly with time.Unix (see the current umoci package in openSUSE which cannot build on 32-bit, or just try running make GOARCH=386 local-validate-build before this patch was applied). However .Unix() will always convert it to int64 which means we don't have to deal with it ourselves. On 64-bit systems this doesn't change anything.

That was the original reason I used Unix and I completely forgot when reviewing your patch. It's my fault that I didn't make this part of the CI when I first figured out this same issue several months ago. Not to worry, it's part of CI now. 😸

@cyphar
Copy link
Member Author

cyphar commented Jul 24, 2017

If you're wondering about what will happen in 2038 to Go programs that use Timespec on a 32-bit machine, your guess is as good as mine. I imagine it won't end well.

@vrothberg
Copy link
Contributor

On 32-bit systems, Timespec.Sec and Timespec.Nsec are int32 [...]

Ha, I should have been more careful as I was aware of the casts, but found accessing the members more explicit. Thanks for the fix and the explanation 👍

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.

2 participants