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

Replace Travis with GitHub actions #1145

Closed
wants to merge 1 commit into from
Closed

Conversation

hoyon
Copy link

@hoyon hoyon commented Mar 26, 2023

I saw in #1115 that one of the main barriers for creating a new release of GLM was the fact that Travis ci is no longer available and a new CI system is required for Linux/macOS so I thought I'd give this a go.

Here's my initial attempt at migrating to using GitHub actions. However there are a few issues that I came across:

  • Seg fault with GCC C++11/14 AVX builds
  • Some tests require C++11 and do not compile with C++98
  • More clang warnings need to be disabled to get clang building
  • The reinterpret_cast in test/core/core_func_integer.cpp needed to be removed to avoid a segfault in gcc builds
  • macOS clang complains about vsprintf so switch to using vsnprintf for C++11 builds.

I haven't been able to fix the gcc C++11 and C++14 AVX builds seg fault issue when running some of the tests. I'm not very familiar with SIMD so was unable to start to figure out what's gone wrong with that code.

I have been able to replicate this on my machine (Linux x86_64; GCC 13; AMD 3900 CPU) with the following commands:

cmake -S . -B build -DGLM_TEST_ENABLE_SIMD_AVX=ON -DGLM_TEST_ENABLE_CXX_11=ON -DCMAKE_BUILD_TYPE=Debug
cmake --build build --parallel
ctest --test-dir build

Output:

...

98% tests passed, 4 tests failed out of 193

Total Test time (real) =   2.48 sec

The following tests FAILED:
	188 - test-perf_matrix_div (SEGFAULT)
	189 - test-perf_matrix_inverse (SEGFAULT)
	190 - test-perf_matrix_mul (SEGFAULT)
	192 - test-perf_matrix_transpose (SEGFAULT)

Example build: https://github.com/hoyon/glm/actions/runs/6658885038/job/18096744066

I've also excluded C++98 as although the README claims support, some of the tests won't compile without at least C++11. If continued support for C++98 is desired, it can be added to the matrix and the offending tests rewritten but my feeling is that no one will complain if the minimum version is raised to C++11.

I've only replaced Travis and not Appveyor in this PR as the latter still appears to work and it's been a long time since I've done any C++ development on Windows.

The fact that these issues were able to slip through shows the importance of having a robust CI system in place.

@ericoporto
Copy link

ericoporto commented Oct 26, 2023

This PR looks very good, I don't think actually fixing the failing CIs is necessary for it because then later PRs could fix these issues, since they are things that are currently broken - in a different scope.

@hoyon hoyon marked this pull request as ready for review October 26, 2023 19:43
@hoyon
Copy link
Author

hoyon commented Oct 26, 2023

Thanks for the nudge @ericoporto! I'd forgotten about this.

I've rebased the PR, ignored more clang warnings and updated the wording.

I see two ways of proceeding with this:

  • merge it in now and work towards making it green in subsequent PRs
  • make the required changes to get it green in other PRs then finally merge this when it's all in

After this is done it would be good to also move the windows builds over to GitHub Actions which I haven't done yet as Appveyor appears to still work.

@christophe-lunarg what do you think?

@christophe-lunarg
Copy link

This issue was resolved with #1167 and the introduction of GitHub actions.

Thanks for contributing!

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