chore: add alt-arch tests to GHA #85
Conversation
lib/env.h
Outdated
| @@ -49,6 +49,8 @@ | |||
| # define BASE64_WORDSIZE __WORDSIZE | |||
| #elif defined (__LONG_WIDTH__) | |||
| # define BASE64_WORDSIZE __LONG_WIDTH__ | |||
| #elif defined(__SIZEOF_LONG__) && defined(__CHAR_BIT__) && (__CHAR_BIT__ == 8) | |||
| # define BASE64_WORDSIZE (__SIZEOF_LONG__ * 8) | |||
There was a problem hiding this comment.
Why sizeof(long) * 8? That makes no sense to me. Also, I think we can assume that __CHAR_BIT__ == 8, unless we expect to run on very very strange and unsupported architectures. I mean, we're writing a bit-twiddling library here.
At the least, the commit message should explain why this change is necessary.
There was a problem hiding this comment.
I think we can assume that CHAR_BIT == 8
If we assume, let's just enforce that the assumption is correct no ?
I amende the commit message.
Why
sizeof(long) * 8
That's 64 on 64-bits arch and 32 on 32-bits arch, same as __LONG_WIDTH__ just above but not defined with clang.
There was a problem hiding this comment.
I think __SIZE_WIDTH__ or __POINTER_WIDTH__ might be more applicable, because size_t and pointers are pretty much always expressed in the native word size.
There was a problem hiding this comment.
I think SIZE_WIDTH or POINTER_WIDTH might be more applicable, because size_t and pointers are pretty much always expressed in the native word size.
not on x86_32
There was a problem hiding this comment.
I don't like that long stuff either but that's what makes the most sense to me right now...
There was a problem hiding this comment.
Alright, fair enough. The only place where BASE64_WORDSIZE is used is to choose between 32-bit and 64-bit generic codecs, so nothing should break if we accidentally pick the wrong one.
There was a problem hiding this comment.
I'm reworking this a bit differently with __SIZE_WIDTH__ & proper detection for x32 ABI in #87.
|
Thanks, not bad, I see shades of my other project here 😸 I was going to take this ticket, but you beat me to it. My plan was to split out the multiarch stuff into a separate workflow file called |
mayeut
force-pushed
the
alt-arch
branch
2 times, most recently
from
7016231
to
642f8b7
Compare
|
I noticed that at least on |
|
Same for e.g. |
I think if we test specific code (I missed NEON64/NEON32 for makefile, will add it) or big-endian, it's still functional testing. But more than that, it's easy to update one workflow and forget the other one in another file so I feel it's less error prone to have them in a single file (given it does not increase its complexity/readablity by much). |
|
Can we remove the Travis CI support in a separate issue? I'd like to separate that concern. Also, some extra commits would need to be made, such as removing the Travis badge from the |
mayeut
force-pushed
the
alt-arch
branch
2 times, most recently
from
c9ec6ac
to
08d8d55
Compare
mayeut
changed the title
| # no AVX2 on GHA macOS | ||
| if [ "$(uname -s)" != "Darwin" ]; then | ||
| export AVX2_CFLAGS=-mavx2 | ||
| fi |
There was a problem hiding this comment.
The exception for macOS can be removed once #82 lands.
The detection for word size was dependent on `__LONG_WIDTH__` for at least gcc Alpine Linux (probably other musl distros too). This is not working with clang which does not define `__LONG_WIDTH__`. Replace `__LONG_WIDTH__` by `__SIZE_WIDTH__`. As this will not work as expected for x32 ABI, add a robust x32 detection first.
Use alpine images rather the debian/ubuntu images that were used on TravisCI as they are much lighter (5.5 MB vs 65 MB). The size of downloaded packages shall be roughly the same in any case.
The old Travis CI config compiled and tested the code on various architectures: arm32, arm64, ppc64be, ppc64le and s390. We can partly replicate this with GitHub Actions by using the `uraimo/run-on-arch-action'. Update the CI config to run and test the library on various architectures. Closes #83. Closes #85.
Add alt-arch tests to GHA
Use alpine images rather the debian/ubuntu images that were used on TravisCI as they are much lighter (5.5 MB vs 65 MB).
The size of downloaded packages shall be roughly the same in any case.
fixes #83
build on top of #87