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

ci: wire up support for Win32 #100

Merged
merged 6 commits into from
Sep 19, 2024
Merged

ci: wire up support for Win32 #100

merged 6 commits into from
Sep 19, 2024

Conversation

pks-t
Copy link
Member

@pks-t pks-t commented Sep 5, 2024

Wire up support for testing against Win32 in our CI. This is done by migrating from Makefiles to CMake, which is preinstalled on all GitHub runner images and thus way easier to use.

@pks-t pks-t force-pushed the pks-ci-win32 branch 7 times, most recently from 863df5d to 59f7920 Compare September 5, 2024 06:59
@pks-t
Copy link
Member Author

pks-t commented Sep 5, 2024

Depends on #99 due to compiler errors with MSYS.

According to gettimeofday(3P), passing a non-NULL timezone pointer to
the function is unspecified behaviour. This is also being warned about
by compilers when compiling with strict C90 standard and without most of
the extensions.

Adapt the code accordingly.
@pks-t pks-t force-pushed the pks-ci-win32 branch 6 times, most recently from 0e05d2e to fc62d24 Compare September 5, 2024 09:16
@pks-t pks-t changed the title WIP: ci: wire up support for Win32 ci: wire up support for Win32 Sep 5, 2024
@pks-t
Copy link
Member Author

pks-t commented Sep 5, 2024

Rebased on top of #99 to fix compilation with MinGW, so things are working now with some additional commits on top.

@pks-t pks-t requested a review from ethomson September 5, 2024 09:18
Copy link

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ethomson
Copy link
Member

ethomson commented Sep 5, 2024

What's the rationale for the uintmax_t change? Is this some WIN32 being ambiguous between MSVC and MinGW or something?

@pks-t
Copy link
Member Author

pks-t commented Sep 5, 2024

@ethomson It's mostly about it not being portable between platforms, and this is getting hit in Windows builds. We could likely work around this somehow with more preprocessor magic. But I think that the more reasonable approach here is to drop it and instead use PRIuMAX and PRIdMAX, which are portable. It saves us some pain and is also conceptually the right approach because it gives us the maximum representable integers.

I've got another patch series queued up where I'll expand on this a bit by introducing type-safe wrappers that always use uintmax_t and intmax_t.

@pks-t pks-t force-pushed the pks-ci-win32 branch 2 times, most recently from 5e1c95f to 3a45c1b Compare September 5, 2024 12:34
@pks-t
Copy link
Member Author

pks-t commented Sep 5, 2024

Fixed the CLAR_FIXTURE_PATH. I forgot to add the resources/ suffix.

@ethomson
Copy link
Member

ethomson commented Sep 5, 2024

It saves us some pain and is also conceptually the right approach because it gives us the maximum representable integers.

I'm not super convinced about the concepts, but I'm also not going to block on a relatively small change.

But if we're aiming for conceptual correctness, then let's change the decl itself to uintmax_t so that we make it consistent and we're not just casting from size_t to uintmax_t.

@pks-t
Copy link
Member Author

pks-t commented Sep 5, 2024

But if we're aiming for conceptual correctness, then let's change the decl itself to uintmax_t so that we make it consistent and we're not just casting from size_t to uintmax_t.

@ethomson Which decls are you referring to specifically? Do you mean the cl_assert_equal_i() and similar ones? We don't actually have a variant for size_t there, only for plain integers.

@ethomson
Copy link
Member

ethomson commented Sep 5, 2024

The formatting directives PRIuZ and PRIxZ are not portable across
platforms and cause issues on Windows. Stop using them and instead use
PRIuMAX and PRIxMAX, which are portable.
@pks-t
Copy link
Member Author

pks-t commented Sep 5, 2024

@ethomson Ah, that one. Adjusted it now 👍

The project uses a Makefile as the build system for the test executable.
This makes it hard to build and test on Windows via MSVC.

Introduce support for CMake.
Convert the CI to use CMake instead of Makefiles. This will allow us to
wire up support for Windows in a subsequent commit.
Now that we have wired up support for CMake it is trivial to also
compile the test binary on Windows. Wire this up.
The Makefile in "test/" has been responsible for building the
"clar_test" executable as a sort of smoke test. Now that we have
introduced support for CMake it has been superseded. Remove it.
@pks-t
Copy link
Member Author

pks-t commented Sep 15, 2024

Rebased with some smallish fixups that I made while working on the clar selftests in #102. @ethomson I consider this PR to be ready now, but please let me know in case you think there's anything missing in here.

@ethomson ethomson merged commit 94461c1 into clar-test:main Sep 19, 2024
5 checks passed
@ethomson
Copy link
Member

Thanks!

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.

3 participants