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

Add Continious Integration via GitHub Actions, and Fix Various Build Problems #168

Merged
merged 6 commits into from
Dec 21, 2024

Conversation

biergaizi
Copy link
Contributor

@biergaizi biergaizi commented Dec 19, 2024

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:

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.

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]>
@thliebig
Copy link
Owner

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 !!!

@thliebig thliebig merged commit faec0f2 into thliebig:master Dec 21, 2024
@thliebig
Copy link
Owner

@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?

@biergaizi
Copy link
Contributor Author

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.

@thliebig
Copy link
Owner

Yeah this seems to be the case... but it makes sense, the PR do not contain the/any CI files yet...

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