-
Notifications
You must be signed in to change notification settings - Fork 160
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
Add Continious Integration via GitHub Actions, and Fix Various Build Problems #168
Conversation
bccbe1d
to
a2d5c53
Compare
The last commit was yet again not completely successful at fixing find_package(VTK). According to CMake documentation, find_package() should continue even if the package is not found, Unfortunately, in practice a nonexistent module like CommonCore causes CMake to fail. Thus, find vtkCommonCore instead - this module exists in both VTK 9 and earlier versions. In the future, it may be removed but at least it works for now. Signed-off-by: Yifeng Li <[email protected]>
HDF5's CMake rules have a bug when running on FreeBSD, making it to believe HDF5 1.10 is older than HDF5 1.8 [1], thus CMake fails. Furthermore, implementing a manual version check is also problematic since it also has a bug on Linux so it doesn't define HDF5_VERSION_MAJOR, worse, HDF5_VERSION is only defined in CMake 3.3 so it won't work on old systems that actually need this check. To simplify the workaround we simply drop the HDF5 version check (since HDF 1.8 was released in 2008, while HDF 1.6 was last updated in 2011, it's unlikely to cause any issue). [1] https://gitlab.kitware.com/cmake/cmake/-/issues/25358 Signed-off-by: Yifeng Li <[email protected]>
The parameter "typename EngineType" in FDTD extension templates is the same as "enum EngineType" from class Engine, causing a name conflict when it's compiled via clang. Rename the template parameter to "EngType" instead. Signed-off-by: Yifeng Li <[email protected]>
"inline static" variables are a C++17 feature. To maintain C++11 compliance, move their definitions from signal.h to signal.c (since global variables can't be defined in the header without violating the One Definition Rule). Signed-off-by: Yifeng Li <[email protected]>
Double parentheses is a common convention for assigning values inside a if (e.g. "if ((retval = f())") while suppressing compiler warnings. On the other hand, using double parentheses for pure comparison creates is also warned by clang: warning: equality comparison with extraneous parentheses [-Wparentheses-equality] Remove superfluous parentheses to fix the warning. The warning itself is harmless, but if a large number of warnings are generated, meaningful warnings become difficult to see. Signed-off-by: Yifeng Li <[email protected]>
This Pull Request introduces Continuous Integration via GitHub Actions on multiple operating systems, including Debian, Debian oldstable, Debian oldoldstable, Fedora, AlmaLinux 8, AlmaLinux 9, and macOS on ARM. To ensure POSIX compatibility, Alpine Linux is tested due to its non-GNU C library, and FreeBSD is tested due to its non-Linux environment. For legacy compatibility, Ubuntu 14.04 and CentOS 7 is also tested (may be dropped in the future). Two simple simulation examples are also used as "smoketests" to ensure Octave and Python scripts can be executed without errors. The simulations are limited to 1000 timesteps, so they can finish almost immediately wihout wasting CPU time. Signed-off-by: Yifeng Li <[email protected]>
Look all good and builds and seems to run fine. Ready to merge I assume? I hope this does not mess/conflicts to hard with your PR #161 ? Thanks again for all your hard work @biergaizi !!! |
@biergaizi We need to enable some (reduced/simplified? version) CI for the pull request? At least we should check if the PR builds I guess? Afais the CI only acts on my pushes? |
I think it doesn't retroactively apply to pre-existing pull requests, but it should be enabled for new Pull Request. But I'm not sure. |
Yeah this seems to be the case... but it makes sense, the PR do not contain the/any CI files yet... |
CI: Add Continuous Integration.
This Pull Request introduces Continuous Integration via GitHub Actions on multiple operating systems, including Debian, Debian oldstable, Debian oldoldstable, Fedora, AlmaLinux 8, AlmaLinux 9, and macOS on ARM. To ensure POSIX compatibility, Alpine Linux is tested due to its non-GNU C library, and FreeBSD is tested due to its non-Linux environment. For legacy compatibility, Ubuntu 14.04 and CentOS 7 is also tested (may be dropped in the future).
Two simple simulation examples are also used as "smoketests" to ensure Octave and Python scripts can be executed without errors. The simulations are limited to 1000 timesteps, so they can finish almost immediately wihout wasting CPU time.
CMakeLists.txt: find VTK on CentOS 7
The last commit was yet again not completely successful at fixing find_package(VTK). According to CMake documentation, find_package() should continue even if the package is not found, Unfortunately, in practice a nonexistent module like CommonCore causes CMake to fail. Thus, find vtkCommonCore instead - this module exists in both VTK 9 and earlier versions. In the future, it may be removed but at least it works for now.
CMakeLists.txt: drop HDF5 version check
HDF5's CMake rules have a bug when running on FreeBSD, making it to believe HDF5 1.10 is older than HDF5 1.8 [1], thus CMake fails. Furthermore, implementing a manual version check is also problematic since it also has a bug on Linux so it doesn't define HDF5_VERSION_MAJOR, worse, HDF5_VERSION is only defined in CMake 3.3 so it won't work on old systems that actually need this check. To simplify the workaround we simply drop the HDF5 version check (since HDF 1.8 was released in 2008, while HDF 1.6 was last updated in 2011, it's unlikely to cause any issue).
[1] https://gitlab.kitware.com/cmake/cmake/-/issues/25358
FDTD: rename "typename EngineType" to "typename EngType
The parameter "typename EngineType" in FDTD extension templates is the same as "enum EngineType" from class Engine, causing a name conflict when it's compiled via clang. Rename the template parameter to "EngType" instead.
tools/signal.h: Fix C++11 compliance
"inline static" variables are a C++17 feature. To maintain C++11 compliance, move their definitions from signal.h to signal.c (since global variables can't be defined in the header without violating the One Definition Rule).
FDTD/operator.cpp: remove superfluous parentheses
Double parentheses is a common convention for assigning values inside a if (e.g. "if ((retval = f())") while suppressing compiler warnings. On the other hand, using double parentheses for pure comparison creates is also warned by clang:
Remove superfluous parentheses to fix the warning.
The warning itself is harmless, but if a large number of warnings are generated, meaningful warnings become difficult to see.