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 CI with GitHub Actions #575

Merged
merged 11 commits into from
Sep 1, 2024

Conversation

andre-schulz
Copy link
Collaborator

@andre-schulz andre-schulz commented Aug 31, 2024

Hi,
this PR replaces the currently broken Travis CI with GitHub Actions. GitHub Actions is GitHub's own CI infrastructure which is available for free for public repositories, quote from 1:

"GitHub Actions usage is free for standard GitHub-hosted runners in public repositories [...]"

Commits 1 (db7f802) to 5 (3eb318d) are fixes/improvements that pave the way for setting up CI via GitHub Actions.

Commit 6 (048e0bb) is the main change which sets up CI via GitHub Actions and includes as many different build environments as possible:

  • Ubuntu 20.04, GCC 9.4.0/Clang 10.0.0, x86_64
  • Ubuntu 22.04, GCC 11.4.0/Clang 14.0.0, x86_64
  • Ubuntu 24.04, GCC 13.2.0/Clang 18.1.3, x86_64
  • macOS 12, Clang 14.0.0, x86_64
  • macOS 13, Clang 15.0.0, x86_64
  • macOS 14, Clang 15.0.0, arm64

Commit 7 (1b693c2) removes the obsolete Travis CI YAML file.

Commits 8 (247ea30) to 11 (1014e8f) fix some compiler warnings to reduce noise in the CI logs.

Footnotes

  1. https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions#about-billing-for-github-actions

gtest, libjpeg/libjpeg-turbo, libpng and libtiff provide pkg-config
files which make compiling and linking against them easier.

This is needed for running CI on macOS with GitHub Actions in a later
commit.
OpenGL on macOS was deprecated in macOS 10.14 [1]. Silence the warnings
because the OpenGL code won't be replaced anytime soon if at all.

[1] https://developer.apple.com/library/archive/documentation/GraphicsImaging/Conceptual/OpenGL-MacProgGuide/opengl_intro/opengl_intro.html
GoogleTest requires C++14 since 1.13.0 (see release notes [1]).

This is required for running CI on macOS with GitHub Actions in a
later commit because Homebrew will install GoogleTest 1.15.2 [2].

[1] https://github.com/google/googletest/releases/tag/v1.13.0
[2] https://formulae.brew.sh/formula/googletest
While setting up GitHub Actions, SurfTest.TestHaarWaveletsLargerKernel
failed in the following CI environments:
- clang version 10.0.0-4ubuntu,             Ubuntu 20.04, x86_64
- Ubuntu clang version 14.0.0-1ubuntu1.1,   Ubuntu 22.04, x86_64
- Ubuntu clang version 18.1.3 (1ubuntu1),   Ubuntu 24.04, x86_64
- Apple clang 14.0.0 (clang-1400.0.29.202), macOS 12,     x86_64
- Apple clang 15.0.0 (clang-1500.0.40.1),   macOS 13,     x86_64

Output of the Ubuntu 20.04 and 22.04 CI runners that failed:
------------------------------------------------------------
[ RUN      ] SurfTest.TestHaarWaveletsLargerKernel
sfm/gtest_surf.cc:151: Failure
Expected equality of these values:
  6.0f
    Which is: 6
  dy
    Which is: 6
[  FAILED  ] SurfTest.TestHaarWaveletsLargerKernel (0 ms)

Output of the Ubuntu 24.04, macOS 12 and 13 CI runners that failed:
-------------------------------------------------------------------
[ RUN      ] SurfTest.TestHaarWaveletsLargerKernel
sfm/gtest_surf.cc:151: Failure
Expected equality of these values:
  6.0f
    Which is: 6
  dy
    Which is: 6.00000048

[  FAILED  ] SurfTest.TestHaarWaveletsLargerKernel (0 ms)

Fixed by replacing EXPECT_EQ with EXPECT_FLOAT_EQ which verifies that
the values are approximately equal within 4 ULPs [1].

[1] https://google.github.io/googletest/reference/assertions.html#floating-point
Behavior of parsing doubles from streams is different in LLVM's libc++
compared to GCC's libstdc++, see [1]. This causes
StringTest.StringConversionTest to fail when compiling against LLVM's
libc++:

  [ RUN      ] StringTest.StringConversionTest
  util/gtest_string.cc:48: Failure
  Expected equality of these values:
    10.1234
    util::string::convert<double>("10.1234asfd", false)
      Which is: 0

  [  FAILED  ] StringTest.StringConversionTest (0 ms)

This mainly affects macOS because it defaults to libc++.

The behavior of libstdc++ is what makes most sense here because
std::stringstream::operator>>() will call std::strtod() internally
which will extract 10.1234 from the string.

This is fixed by implementing a workaround for libc++.

[1] https://www.github.com/llvm/llvm-project/issues/18156
The Travis CI build in this repository no longer works but GitHub provides
its own CI/CD infrastructure which is free for public repositories, quote
from [1]:

  "GitHub Actions usage is free for standard GitHub-hosted runners in public
   repositories [...]"

The list of standard GitHub-hosted runners for public repositories can
be found at the following URL:

  https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories

[1] https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions
Travis CI has been replaced by GitHub Actions.
Apparently, vsprintf() is deprecated on macOS. Fixes the following
compiler warning when compiling with Apple clang 14.0.0 (clang-1400.0.29.202)
on macOS 12:

  clang++ -Wall -Wextra -Wundef -pedantic -march=native -funsafe-math-optimizations -fno-math-errno -std=c++14 -g -O3 -pthread -DGL_SILENCE_DEPRECATION=1 -fPIC -I../../libs `pkg-config --cflags libjpeg` `pkg-config --cflags libpng` `pkg-config --cflags libtiff-4`  -c -o image_io.o image_io.cc
  image_io.cc:634:7: warning: 'vsprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use vsnprintf(3) instead. [-Wdeprecated-declarations]
      ::vsprintf(msg, fmt, ap);
        ^
  /Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/stdio.h:207:1: note: 'vsprintf' has been explicitly marked deprecated here
Found by Apple clang version 14.0.0 (clang-1400.0.29.202) on macOS 12:

  bundle_io.cc:743:9: warning: variable 'num_points_3d' set but not used [-Wunused-but-set-variable]
…dNormal()

Found by Clang 17.0.6 on Gentoo Linux:

  patch_optimization.cc:316:17: warning: variable 'row' set but not used [-Wunused-but-set-variable]
    316 |     std::size_t row = 0;
        |                 ^
@andre-schulz
Copy link
Collaborator Author

andre-schulz commented Aug 31, 2024

When compiling on Ubuntu 20.04 with Clang 10.0.0, the following warning shows up multiple times when compiling unit tests:

clang++ -std=c++14 -pthread -Wall -Wextra -pedantic -Wno-sign-compare -I../libs `pkg-config --cflags gtest_main`  -c -o math/gtest_algo.o math/gtest_algo.cc
clang: warning: -lpthread: 'linker' input unused [-Wunused-command-line-argument]

The warning no longer shows up with newer Clang versions, e.g. Ubuntu 22.04 with Clang 14.0.0, so I assume that this is an issue that has been fixed in Clang.

I could modify the build system to detect the specific Clang version to silence the warning but I'm not sure if this is worth the effort because Clang 10.0.0 is rather old and Ubuntu 20.04 standard support ends on April 2025 1. If you want I can look into this or we could fix the issue in a follow-up PR. What do you think?

Footnotes

  1. https://ubuntu.com/about/release-cycle

@andre-schulz
Copy link
Collaborator Author

andre-schulz commented Aug 31, 2024

When compiling on macOS 12/13/14 with Clang, the following warning shows up multiple times during compilation of the tests:

  clang++ -std=c++14 -pthread -Wall -Wextra -pedantic -Wno-sign-compare -I../libs `pkg-config --cflags gtest_main`  -c -o math/gtest_functions.o math/gtest_functions.cc
  math/gtest_accum.cc:10:1: warning: use of the 'maybe_unused' attribute is a C++17 extension [-Wc++17-extensions]
  TEST(AccumTest, TestFloatAccum)
  ^
  /usr/local/Cellar/googletest/1.15.2/include/gtest/gtest.h:2192:42: note: expanded from macro 'TEST'
  #define TEST(test_suite_name, test_name) GTEST_TEST(test_suite_name, test_name)
                                           ^
  /usr/local/Cellar/googletest/1.15.2/include/gtest/gtest.h:2186:3: note: expanded from macro 'GTEST_TEST'
    GTEST_TEST_(test_suite_name, test_name, ::testing::Test, \
    ^

I can also reproduce this on Gentoo Linux with Clang 18.1.8 and GoogleTest 1.15.2.

It looks like that this is an issue with GoogleTest and how it determines whether a C++ attribute is supported by the compiler and the chosen C++ standard to compile with, see GoogleTest issue 4605.

I see two ways to fix this issue:

  1. Do nothing and just wait it out until GoogleTest fixes the issue.
  2. Raise the C++ version to C++17 in MVE.

What do you think?

@andre-schulz
Copy link
Collaborator Author

When compiling UMVE on Ubuntu 22.04/24.04 with GCC/Clang, the following warning shows up multiple times:

  In file included from /usr/include/x86_64-linux-gnu/qt5/QtGui/qopengl.h:146,
                   from /usr/include/x86_64-linux-gnu/qt5/QtWidgets/qopenglwidget.h:49,
                   from /usr/include/x86_64-linux-gnu/qt5/QtWidgets/QOpenGLWidget:1,
                   from glwidget.h:14,
                   from glwidget.cc:16:
  /usr/include/x86_64-linux-gnu/qt5/QtGui/qopenglext.h:60: warning: "GL_GLEXT_VERSION" redefined
     60 | #define GL_GLEXT_VERSION 20190228
        | 
  In file included from /usr/include/GL/gl.h:2050,
                   from ../../libs/ogl/opengl.h:29,
                   from glwidget.cc:14:
  /usr/include/GL/glext.h:35: note: this is the location of the previous definition
     35 | #define GL_GLEXT_VERSION 20220530
        | 

The problem is that Qt ships its own glext.h, qopenglext.h in QtGui, which has a different header guard (__glext_h_) than the system glext.h (__gl_glext_h_).

Judging from a post in the Qt forum, It looks like this issue has been present since at least December 2020. Looking at qopenglext.h in the official Qt repo, the issue is still present in the latest 5.15 branch and the latest dev branch.

A possible workaround for this could be the following:

diff --git a/libs/ogl/opengl.h b/libs/ogl/opengl.h
index 397388bd72a5..4fb5d95ea67d 100644
--- a/libs/ogl/opengl.h
+++ b/libs/ogl/opengl.h
@@ -27,7 +27,12 @@
 #else
 #  define GL_GLEXT_PROTOTYPES
 #  include <GL/gl.h>
-#  include <GL/glext.h>
+#  if !defined(QT_OPENGL_LIB) || (defined(QT_OPENGL_LIB) && !defined(__glext_h_))
+#    include <GL/glext.h>
+#    if defined(QT_OPENGL_LIB)
+#      define __glext_h_ 1
+#    endif
+#  endif
 #endif

 #endif /* OGL_OPEN_GL_HEADER */

Unfortunately, QT_OPENGL_LIB is undocumented but this fixes the warnings. What do you think?

@simonfuhrmann
Copy link
Owner

When compiling on Ubuntu 20.04 with Clang 10.0.0, the following warning shows up multiple times when compiling unit tests:

clang++ -std=c++14 -pthread -Wall -Wextra -pedantic -Wno-sign-compare -I../libs `pkg-config --cflags gtest_main`  -c -o math/gtest_algo.o math/gtest_algo.cc
clang: warning: -lpthread: 'linker' input unused [-Wunused-command-line-argument]

The warning no longer shows up with newer Clang versions, e.g. Ubuntu 22.04 with Clang 14.0.0, so I assume that this is an issue that has been fixed in Clang.

I could modify the build system to detect the specific Clang version to silence the warning but I'm not sure if this is worth the effort because Clang 10.0.0 is rather old and Ubuntu 20.04 standard support ends on April 2025 1. If you want I can look into this or we could fix the issue in a follow-up PR. What do you think?

Footnotes

  1. https://ubuntu.com/about/release-cycle

Agree, probably not worth the effort.

@simonfuhrmann
Copy link
Owner

When compiling on macOS 12/13/14 with Clang, the following warning shows up multiple times during compilation of the tests:

  clang++ -std=c++14 -pthread -Wall -Wextra -pedantic -Wno-sign-compare -I../libs `pkg-config --cflags gtest_main`  -c -o math/gtest_functions.o math/gtest_functions.cc
  math/gtest_accum.cc:10:1: warning: use of the 'maybe_unused' attribute is a C++17 extension [-Wc++17-extensions]
  TEST(AccumTest, TestFloatAccum)
  ^
  /usr/local/Cellar/googletest/1.15.2/include/gtest/gtest.h:2192:42: note: expanded from macro 'TEST'
  #define TEST(test_suite_name, test_name) GTEST_TEST(test_suite_name, test_name)
                                           ^
  /usr/local/Cellar/googletest/1.15.2/include/gtest/gtest.h:2186:3: note: expanded from macro 'GTEST_TEST'
    GTEST_TEST_(test_suite_name, test_name, ::testing::Test, \
    ^

I can also reproduce this on Gentoo Linux with Clang 18.1.8 and GoogleTest 1.15.2.

It looks like that this is an issue with GoogleTest and how it determines whether a C++ attribute is supported by the compiler and the chosen C++ standard to compile with, see GoogleTest issue 4605.

I see two ways to fix this issue:

  1. Do nothing and just wait it out until GoogleTest fixes the issue.
  2. Raise the C++ version to C++17 in MVE.

What do you think?

Let's sit it out. It's just a warning, and only for testing, so not that critical.

@simonfuhrmann
Copy link
Owner

When compiling UMVE on Ubuntu 22.04/24.04 with GCC/Clang, the following warning shows up multiple times:

  In file included from /usr/include/x86_64-linux-gnu/qt5/QtGui/qopengl.h:146,
                   from /usr/include/x86_64-linux-gnu/qt5/QtWidgets/qopenglwidget.h:49,
                   from /usr/include/x86_64-linux-gnu/qt5/QtWidgets/QOpenGLWidget:1,
                   from glwidget.h:14,
                   from glwidget.cc:16:
  /usr/include/x86_64-linux-gnu/qt5/QtGui/qopenglext.h:60: warning: "GL_GLEXT_VERSION" redefined
     60 | #define GL_GLEXT_VERSION 20190228
        | 
  In file included from /usr/include/GL/gl.h:2050,
                   from ../../libs/ogl/opengl.h:29,
                   from glwidget.cc:14:
  /usr/include/GL/glext.h:35: note: this is the location of the previous definition
     35 | #define GL_GLEXT_VERSION 20220530
        | 

The problem is that Qt ships its own glext.h, qopenglext.h in QtGui, which has a different header guard (__glext_h_) than the system glext.h (__gl_glext_h_).

Judging from a post in the Qt forum, It looks like this issue has been present since at least December 2020. Looking at qopenglext.h in the official Qt repo, the issue is still present in the latest 5.15 branch and the latest dev branch.

A possible workaround for this could be the following:

diff --git a/libs/ogl/opengl.h b/libs/ogl/opengl.h
index 397388bd72a5..4fb5d95ea67d 100644
--- a/libs/ogl/opengl.h
+++ b/libs/ogl/opengl.h
@@ -27,7 +27,12 @@
 #else
 #  define GL_GLEXT_PROTOTYPES
 #  include <GL/gl.h>
-#  include <GL/glext.h>
+#  if !defined(QT_OPENGL_LIB) || (defined(QT_OPENGL_LIB) && !defined(__glext_h_))
+#    include <GL/glext.h>
+#    if defined(QT_OPENGL_LIB)
+#      define __glext_h_ 1
+#    endif
+#  endif
 #endif

 #endif /* OGL_OPEN_GL_HEADER */

Unfortunately, QT_OPENGL_LIB is undocumented but this fixes the warnings. What do you think?

I've seen this warning, too. What concerns me a bit is that the versions are different (20190228 vs 20220530). I do not know, however, what the impact of this warning is. So I don't mind if we leave the warning, if things work. Not a fan of warnings, but I don't know what to do, haha.

@simonfuhrmann
Copy link
Owner

First of all, thank you Andre, this PR is great! Very much appreciated.

@simonfuhrmann simonfuhrmann marked this pull request as ready for review September 1, 2024 23:52
@simonfuhrmann simonfuhrmann merged commit 24a3f7a into simonfuhrmann:master Sep 1, 2024
@simonfuhrmann
Copy link
Owner

When clicking on the chip in README.md that points to https://github.com/simonfuhrmann/mve/actions/workflows/main.yml, I'll get a 404. Is there a way to make it point to the progress or the last results of the CI?

@simonfuhrmann
Copy link
Owner

simonfuhrmann commented Sep 1, 2024

When clicking on the chip in README.md that points to https://github.com/simonfuhrmann/mve/actions/workflows/main.yml, I'll get a 404. Is there a way to make it point to the progress or the last results of the CI?

It seems I had to enable Actions in the project repo settings. Still not sure if things are set up correctly.

@andre-schulz
Copy link
Collaborator Author

When clicking on the chip in README.md that points to https://github.com/simonfuhrmann/mve/actions/workflows/main.yml, I'll get a 404. Is there a way to make it point to the progress or the last results of the CI?

It seems I had to enable Actions in the project repo settings. Still not sure if things are set up correctly.

Maybe a new commit has to be pushed to master to trigger a CI run? I just noticed a minor issue with one of my commits in this PR. I'll create a PR for the fix and in the PR we'll see whether the CI stuff is setup correctly.

@andre-schulz
Copy link
Collaborator Author

Looking at the new PR I just created, everything is setup correctly! 🙂

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