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.

Sign up for GitHub

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

Jump to bottom

CI: add tests for various MSYS2 environments #132

Closed
wants to merge 3 commits into from
Closed

CI: add tests for various MSYS2 environments #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24ab1ad
#127: bin/base64: fix compilation on MinGW
aklomp Jan 2, 2024
b07eb1d
chore(ci): add MSYS2 cmake tests
mayeut Jan 6, 2024
b2c8ca4
fix: makefile builds with mingw
mayeut Jan 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 74 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,3 +169,77 @@ jobs:
echo "::group::CTest"
ctest --no-tests=error --test-dir out -VV --build-config Release
echo "::endgroup::CTest"

msys2-cmake-test:
name: msys2-cmake-${{ matrix.msystem }}
needs: makefile-analysis
runs-on: windows-2022
strategy:
fail-fast: false
matrix:
include:
- { msystem: msys, toolchain: "gcc" }
- { msystem: mingw32, env: mingw-w64-i686- }
- { msystem: mingw64, env: mingw-w64-x86_64- }
- { msystem: ucrt64, env: mingw-w64-ucrt-x86_64- }
- { msystem: clang32, env: mingw-w64-clang-i686- }
- { msystem: clang64, env: mingw-w64-clang-x86_64- }

steps:
- name: Checkout
uses: actions/checkout@v3
- name: Setup MSYS2 ${{matrix.msystem}}
uses: msys2/setup-msys2@v2
with:
msystem: ${{matrix.msystem}}
update: true
install: >-
make
${{ matrix.env }}${{ matrix.toolchain || 'toolchain' }}
${{ matrix.env }}cmake
- name: CMake Configure
shell: msys2 {0}
run: >
cmake
-B out
-Werror=dev
-DBASE64_BUILD_TESTS=ON
-DCMAKE_BUILD_TYPE=Release
-DBASE64_WITH_AVX512=OFF
- name: CMake Build
shell: msys2 {0}
run: cmake --build out --config Release --verbose
- name: CTest
shell: msys2 {0}
run: ctest --no-tests=error --test-dir out -VV --build-config Release

msys2-makefile-test:
name: msys2-makefile-${{ matrix.msystem }}
needs: makefile-analysis
runs-on: windows-2022
strategy:
fail-fast: false
matrix:
include:
- { msystem: msys, toolchain: "gcc" }
- { msystem: mingw32, env: mingw-w64-i686- }
- { msystem: mingw64, env: mingw-w64-x86_64- }
- { msystem: ucrt64, env: mingw-w64-ucrt-x86_64- }
# - { msystem: clang32, env: mingw-w64-clang-i686- } disabled, lld does not support the "-r" option
# - { msystem: clang64, env: mingw-w64-clang-x86_64- } disabled, lld does not support the "-r" option
Copy link
Owner

Choose a reason for hiding this comment

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

Does it support -i or --relocatable? Both are synonyms of -r.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The feature is supported for ELF & Mach-O targets but not for Windows.

env:
CC: cc.exe
steps:
- name: Checkout
uses: actions/checkout@v3
- name: Setup MSYS2 ${{matrix.msystem}}
uses: msys2/setup-msys2@v2
with:
msystem: ${{matrix.msystem}}
update: true
install: >-
make
${{ matrix.env }}${{ matrix.toolchain || 'toolchain' }}
- name: Run tests
shell: msys2 {0}
run: ./test/ci/test.sh
19 changes: 14 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
CFLAGS += -std=c99 -O3 -Wall -Wextra -pedantic
CFLAGS += -std=c99 -O3 -Wall -Wextra -pedantic -DBASE64_STATIC_DEFINE

# Set OBJCOPY if not defined by environment:
OBJCOPY ?= objcopy
Expand Down Expand Up @@ -56,6 +56,8 @@ ifdef OPENMP
CFLAGS += -fopenmp
endif

TARGET := $(shell $(CC) -dumpmachine)


.PHONY: all analyze clean

Expand All @@ -64,9 +66,16 @@ all: bin/base64 lib/libbase64.o
bin/base64: bin/base64.o lib/libbase64.o
$(CC) $(CFLAGS) -o $@ $^

lib/libbase64.o: $(OBJS)
$(LD) -r -o $@ $^
$(OBJCOPY) --keep-global-symbols=lib/exports.txt $@
lib/exports.build.txt: lib/exports.txt
ifeq (i686-w64-mingw32, $(TARGET))
sed -e 's/^/_/' $< > $@
else
cp -f $< $@
endif
Copy link
Owner

Choose a reason for hiding this comment

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

Honestly I'm not a big fan of this compatibility block, though I understand why it's needed. In the longer term I think we should try to get rid of the exports file entirely and rely on compiler/linker features to mark functions as hidden or visible.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean like annotating these symbols as BASE64_SYMBOL_PRIVATE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this either but if we're to keep the current semantic, it's needed.
It's not the same as BASE64_SYMBOL_PRIVATE:
BASE64_SYMBOL_PRIVATE only acts on the visibility attribute of GLOBAL symbols e.g. even if base64_stream_encode_plain is marked as hidden it's still linkable (if building a static library or bare object).
--keep-global-symbols acts on LOCAL/GLOBAL attribute. It effectively marks symbols that are not in the list as LOCAL & those are not linkable anymore (as if we only had 1 translation unit - simulated by ld -r - and the function was declared static).

I wouldn't mind if the makefile produced the same things as the CMake build does for a static library (just something like ar -o libbase64.a *.o, getting rid of the ld -r and objcopy).

Another option is not to support makefile builds for Windows at all.

Copy link
Owner

@aklomp aklomp Jan 8, 2024 •
edited

Choose a reason for hiding this comment

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

My idea for the future roadmap is basically to engineer the library such that all functions start with akbase64_ (which is probably what I will rename this library to, see #103), and that all functions are declared static, except for those that are part of the public API. Those two measures should remove all potential naming conflicts in practice. Then we can also get rid of the exports file.

Until then, I suggest we keep going as usual, so using this Makefile hack/complication out of necessity.


lib/libbase64.o: lib/exports.build.txt $(OBJS)
$(LD) -r -o $@ $(OBJS)
$(OBJCOPY) --keep-global-symbols=$< $@

lib/config.h:
@echo "#define HAVE_AVX512 $(HAVE_AVX512)" > $@
Expand Down Expand Up @@ -97,4 +106,4 @@ analyze: clean
scan-build --use-analyzer=`which clang` --status-bugs make

clean:
rm -f bin/base64 bin/base64.o lib/libbase64.o lib/config.h $(OBJS)
rm -f bin/base64 bin/base64.o lib/libbase64.o lib/config.h lib/exports.build.txt $(OBJS)
72 changes: 70 additions & 2 deletions bin/base64.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,19 @@
#define _XOPEN_SOURCE // IOV_MAX
// Test for MinGW.
#if defined(__MINGW32__) || defined(__MINGW64__)
# define MINGW
#endif

// Decide if the writev(2) system call needs to be emulated as a series of
// write(2) calls. At least MinGW does not support writev(2).
#ifdef MINGW
# define EMULATE_WRITEV
#endif

// Include the necessary system header when using the system's writev(2).
#ifndef EMULATE_WRITEV
# define _XOPEN_SOURCE // Unlock IOV_MAX
# include <sys/uio.h>
#endif

#include <stdbool.h>
#include <stdlib.h>
Expand All @@ -8,7 +23,7 @@
#include <getopt.h>
#include <errno.h>
#include <limits.h>
#include <sys/uio.h>

#include "../include/libbase64.h"

// Size of the buffer for the "raw" (not base64-encoded) data in bytes.
Expand Down Expand Up @@ -50,6 +65,59 @@ struct buffer {
char *enc;
};

// Optionally emulate writev(2) as a series of write calls.
#ifdef EMULATE_WRITEV

// Quick and dirty definition of IOV_MAX as it is probably not defined.
#ifndef IOV_MAX
# define IOV_MAX 1024
#endif

// Quick and dirty definition of this system struct, for local use only.
struct iovec {

// Opaque data pointer.
void *iov_base;

// Length of the data in bytes.
size_t iov_len;
};

static ssize_t
writev (const int fd, const struct iovec *iov, int iovcnt)
{
ssize_t r, nwrite = 0;

// Reset the error marker.
errno = 0;

while (iovcnt-- > 0) {

// Write the vector; propagate errors back to the caller. Note
// that this loses information about how much vectors have been
// successfully written, but that also seems to be the case
// with the real function. The API is somewhat flawed.
if ((r = write(fd, iov->iov_base, iov->iov_len)) < 0) {
return r;
}

// Update the total write count.
nwrite += r;

// Return early after a partial write; the caller should retry.
if ((size_t) r != iov->iov_len) {
break;
}

// Move to the next vector.
iov++;
}

return nwrite;
}

#endif // EMULATE_WRITEV

static bool
buffer_alloc (const struct config *config, struct buffer *buf)
{
Expand Down
6 changes: 5 additions & 1 deletion test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
CFLAGS += -std=c99 -O3 -Wall -Wextra -pedantic
CFLAGS += -std=c99 -O3 -Wall -Wextra -pedantic -DBASE64_STATIC_DEFINE
ifdef OPENMP
CFLAGS += -fopenmp
endif

TARGET := $(shell $(CC) -dumpmachine)
ifneq (, $(findstring darwin, $(TARGET)))
BENCH_LDFLAGS=
else ifneq (, $(findstring mingw, $(TARGET)))
BENCH_LDFLAGS=
else
# default to linux, -lrt needed
BENCH_LDFLAGS=-lrt
Expand All @@ -15,7 +17,9 @@ endif

test: clean test_base64 benchmark
./test_base64
ifeq (, $(findstring mingw, $(TARGET))) # no "/dev/urandom" on Windows
Copy link
Owner

Choose a reason for hiding this comment

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

There's a really old pull request, #6, which fixes this on Windows. Now that we have a provisional CI environment for Windows, I can try to revive that branch and see if I can get the benchmark to work.

Having a working Windows benchmark would also be great for checking in CI that certain changes did not introduce speed regressions on that platform.

./benchmark
endif

valgrind: clean test_base64
valgrind --error-exitcode=2 ./test_base64
Expand Down