Modernize CMake #99

Curve · 2023-11-21T17:33:58Z

Features

Incorporate CPM.cmake

Removes need for "_external" targets

Restructure the CMakeLists.txt

Cleans up superfluous options passed to VTK (https://gitlab.kitware.com/vtk/vtk/-/blob/master/Documentation/docs/build_instructions/build_settings.md)

VTK has been updated to 9.3.1 (Pre-Release, but also accepts system installations of VTK>=9.0)

Adds Sanitizer Options for GCC / MSVC

Additional Target is generated for the static / shared library (ViennaLS::Lib)

Various other refactoring

Automatically copies required VTK Dlls to specific targets on Windows when VTK is built from source (No need to manually update the path anymore)

Fix some shellcheck warnings in format-project.sh
Add missing <cstdint> include in lsPointData.hpp

To Discuss

I have currently incorporated the whole CPM.cmake file (which is the recommended installation method), however we could also use the get_cpm script which downloads the specified CPM version once and then reuses it
- ~~This has the drawback that full offline builds will get more difficult~~
The folders Python, Examples and Tests use different casing than the other folders, should we consider renaming them to all lower case?
The doxygen documentation is present in the root folder, maybe we should consider removing it from git¹ and instead let the user generate it on demand?
ViennaHRLE has no tags currently - If it had tags we could handle the versioning of it as a dependency better
It is common practice to have all headers reside in a sub-directory, i.e. <ls/advect.hpp> instead of <lsAdvect.hpp>, which would also simplify the install target slightly, maybe we should consider moving the headers into a sub-directory and therefore remove their ls prefix?

¹ or have it in a dedicated Github-Pages branch instead of master

Breaking / Important Changes

Disabled Sanitizer by default (now opt-in)

Things I've tested

All Tests pass
Examples build fine
Python Bindings still work as expected
System Installations of dependencies are preferred (Tested with VTK and pybind11)
Static build works properly

…ilable

Curve · 2023-11-27T14:36:12Z

Does the Code Style step use cmake-format?

tobre1 · 2023-11-27T14:37:58Z

Does the Code Style step use cmake-format?

Yes, I think cmake-format is used on all CMake files.

Curve · 2023-11-27T14:39:42Z

Does the Code Style step use cmake-format?

Yes, I think cmake-format is used on all CMake files.

Oh, sorry I somehow missed that, I'll check how it looks when formatted with cmake-format, depending on the outcome I might tweak it's config a little (I'm mostly not too much of a fan of cmake-formats output by default because it makes some parts less readable imo)

Edit: Also interesting that the Linux build fails due to missing OpenGL, ~~curious if the update to VTK 9.3.0 caused that~~ I probably missed a flag used in the old build

tobre1 · 2023-11-27T14:43:23Z

Does the Code Style step use cmake-format?

Yes, I think cmake-format is used on all CMake files.

Oh, sorry I somehow missed that, I'll check how it looks when formatted with cmake-format, depending on the outcome I might tweak it's config a little (I'm mostly not too much of a fan of cmake-formats output by default because it makes some parts less readable IMO)

If you refactored the whole CMake file with CPM in a nicely readable way, we could also consider dropping the cmake-format coding style condition. Check if the cmake-format output is very off, otherwise you could still apply it to the CMakeLists file.

Curve · 2023-11-27T14:46:35Z

Does the Code Style step use cmake-format?

Yes, I think cmake-format is used on all CMake files.

Oh, sorry I somehow missed that, I'll check how it looks when formatted with cmake-format, depending on the outcome I might tweak it's config a little (I'm mostly not too much of a fan of cmake-formats output by default because it makes some parts less readable IMO)

If you refactored the whole CMake file with CPM in a nicely readable way, we could also consider dropping the cmake-format coding style condition. Check if the cmake-format output is very off, otherwise you could still apply it to the CMakeLists file.

Will do 👍

Curve · 2023-11-27T21:47:09Z

CMake-Format	Mine

I find the right one a little more readable - but I think both are fine - Let me know which one you prefer

I've also removed the OpenGL dependency from VTK and updated the GitHub workflows and Readme a little to accommodate for the new CMake changes & Fixed the ctest command to properly ignore Benchmarks/Performance Tests (Not sure if this is something that was related to older CTest versions but -LE seems to an invalid flag, using only -E works on my system and in a docker container running Fedora 38)

tobre1 · 2023-11-29T07:41:58Z

I think CMake-Format version also looks fine in this case. This way it might also be easier for other people to adhere to the coding style. Could then please update your code with the format-project.sh script?

Curve · 2023-11-29T07:44:03Z

I think CMake-Format version also looks fine in this case. This way it might also be easier for other people to adhere to the coding style. Could then please update your code with the format-project.sh script?

Will do!

Curve · 2023-11-29T11:49:13Z

Curiously some of the tests (FileWriter, Serialize and VisualizationMesh) failed because the sanitizer found some errors:

Direct leak of 112 byte(s) in 1 object(s) allocated from:
    #0 0x7f886b4e2002 in operator new(unsigned long) /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_new_delete.cpp:95
    #1 0x7f8863805776 in vtkIdList::New() sources/ViennaLS/build/_deps/vtk-src/Common/Core/vtkIdList.cxx:8
    #2 0x555a45c11f92 in lsWriteVisualizationMesh<float, 3>::removeDuplicatePoints(vtkSmartPointer<vtkPolyData>&, double) sources/ViennaLS/include/lsWriteVisualizationMesh.hpp:92
    #3 0x555a45bf0317 in lsWriteVisualizationMesh<float, 3>::apply() sources/ViennaLS/include/lsWriteVisualizationMesh.hpp:705
    #4 0x555a45be1114 in main sources/ViennaLS/Tests/VisualizationMesh/VisualizationMesh.cpp:75
    #5 0x7f8860645ccf  (/usr/lib/libc.so.6+0x27ccf) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)

Indirect leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f886b4e2182 in operator new[](unsigned long) /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_new_delete.cpp:98
    #1 0x7f8863805d43 in vtkIdList::AllocateInternal(long long, long long) sources/ViennaLS/build/_deps/vtk-src/Common/Core/vtkIdList.cxx:63
    #2 0x7f88638063d8 in vtkIdList::SetNumberOfIds(long long) sources/ViennaLS/build/_deps/vtk-src/Common/Core/vtkIdList.cxx:84
    #3 0x555a45bf1776 in void vtkCellArray_detail::GetCellAtIdImpl::operator()<vtkCellArray::VisitState<vtkTypeInt64Array> >(vtkCellArray::VisitState<vtkTypeInt64Array>&, long long, vtkIdList*) sources/ViennaLS/build/_deps/vtk-src/Common/DataModel/vtkCellArray.h:1447
    #4 0x555a45be6cc1 in void vtkCellArray::Visit<vtkCellArray_detail::GetCellAtIdImpl, long long&, vtkIdList*&, void>(vtkCellArray_detail::GetCellAtIdImpl&&, long long&, vtkIdList*&) sources/ViennaLS/build/_deps/vtk-src/Common/DataModel/vtkCellArray.h:969
    #5 0x555a45be4f26 in vtkCellArray::GetCellAtId(long long, vtkIdList*) sources/ViennaLS/build/_deps/vtk-src/Common/DataModel/vtkCellArray.h:1579
    #6 0x555a45be4de9 in vtkCellArray::GetNextCell(vtkIdList*) sources/ViennaLS/build/_deps/vtk-src/Common/DataModel/vtkCellArray.h:1548
    #7 0x555a45c120dc in lsWriteVisualizationMesh<float, 3>::removeDuplicatePoints(vtkSmartPointer<vtkPolyData>&, double) sources/ViennaLS/include/lsWriteVisualizationMesh.hpp:94
    #8 0x555a45bf0317 in lsWriteVisualizationMesh<float, 3>::apply() sources/ViennaLS/include/lsWriteVisualizationMesh.hpp:705
    #9 0x555a45be1114 in main sources/ViennaLS/Tests/VisualizationMesh/VisualizationMesh.cpp:75
    #10 0x7f8860645ccf  (/usr/lib/libc.so.6+0x27ccf) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)

SUMMARY: AddressSanitizer: 144 byte(s) leaked in 2 allocation(s).

We should probably investigate those further

Curve · 2023-11-29T11:51:33Z

.github/workflows/windows_test.yml

@@ -30,22 +30,24 @@ jobs:
          key: viennals-dependency-cache-${{ runner.os }}-${{env.BUILD_TYPE}}-${{ hashFiles( './external/upstream/**CMakeLists.txt' ) }}


https://github.com/cpm-cmake/CPM.cmake?tab=readme-ov-file#cpm_source_cache

We should use the CPM_SOURCE_CACHE in the workflows to cache

Curve · 2023-11-29T14:39:13Z

I've disabled the sanitizer by default currently - To see if the functionality works as intended.

Let me know if we should re-enable the sanitizer by default for debug builds or leave it as opt-in.
Maybe we could have separate workflows that test everything with the sanitizer instead of having it baked into the test workflows? (Not sure on this though)

I could also simplify the workflows a little by using actions to move steps that are commonly duplicated into a single action that can be re-used but I'd rather make a separate PR for that ^^

Edit: Also, we could add a linux workflow that installs vtk (and maybe others as well, for example pybind) as a system dependency to make sure that CPM finds the system dependencies correctly but mainly just so that we can have quicker feedback on the workflow test runs

tobre1 · 2023-11-29T14:53:36Z

I've disabled the sanitizer by default currently - To see if the functionality works as intended.

Let me know if we should re-enable the sanitizer by default for debug builds or leave it as opt-in. Maybe we could have separate workflows that test everything with the sanitizer instead of having it baked into the test workflows? (Not sure on this though)

I think it is good to have the sanitizer on for the tests. But since there are currently some issues with the tests, we could disable it for now and then enable it later when everything is fixed.

I could also simplify the workflows a little by using actions to move steps that are commonly duplicated into a single action that can be re-used but I'd rather make a separate PR for that ^^

That would be nice ^^

Edit: Also, we could add a linux workflow that installs vtk (and maybe others as well, for example pybind) as a system dependency to make sure that CPM finds the system dependencies correctly but mainly just so that we can have quicker feedback on the workflow test runs

Normally, the vtk build should be cached such that the dependencies can be set up quickly. But I am not sure if this only works on workflows on the master branch.
If it is possible to have vtk as system dependency on the Github runner it would probably be quicker than caching a vtk build, I guess?

Also, currently, CPM looks specifically for VTK 9.3.0. Since I only have VTK 9.1.0 installed on my system, it did not use the system installation but downloaded and built version 9.3.0. Since we are not using any super recent VTK functionalities, it would definitely be enough to use any VTK version >9.0. Is it possible for CPM to look for any VTK version >9.0?

Curve · 2023-11-29T14:55:36Z

Normally, the vtk build should be cached such that the dependencies can be set up quickly. But I am not sure if this only works on workflows on the master branch.
If it is possible to have vtk as system dependency on the Github runner it would probably be quicker than caching a vtk build, I guess?

The caching is currently broken due to the CPM switch - I'll fix it and then we can just rely on that

Also, currently, CPM looks specifically for VTK 9.3.0. Since I only have VTK 9.1.0 installed on my system, it did not use the system installation but downloaded and built version 9.3.0. Since we are not using any super recent VTK functionalities, it would definitely be enough to use any VTK version >9.0. Is it possible for CPM to look for any VTK version >9.0?

I'll look into it!

Curve · 2023-11-29T14:56:11Z

Also, the Windows workflow is currently failing because of this upstream issue: https://gitlab.kitware.com/vtk/vtk/-/issues/19166

So it should be fixed by upgrading to vtk 9.3.1 (as soon as that's available)

* This should also fix the windows workflows as we've switched to a commit that includes upstream!10542

Curve · 2023-11-29T15:02:38Z

Also, currently, CPM looks specifically for VTK 9.3.0. Since I only have VTK 9.1.0 installed on my system, it did not use the system installation but downloaded and built version 9.3.0. Since we are not using any super recent VTK functionalities, it would definitely be enough to use any VTK version >9.0. Is it possible for CPM to look for any VTK version >9.0?

This should be fixed now, let me know if it works on your machine (And let's hope the windows workflow properly builds now ^^)

Edit: Seems like the tests build properly on windows now, however it has trouble finding some DLLs, unfortunately the CLI output does not indicate which ones, so I guess I'll have to set this up in a VM on the Weekend

* early return behavior in macro may vary in different cmake versions

Curve added 12 commits November 20, 2023 20:28

refactor(cmake): add CPM.cmake, restructure CMakeLists

573b4c0

refactor(cmake): update examples

028c686

refactor(cmake): update tests

54fbd01

fix: missing include

257e641

refactor(cmake): update python bindings

bdd32ce

feat: add sanitizer options

0d24ce7

feat(cmake): add install target

b152b49

refactor(cmake/python): use CMAKE_LIBRARY_OUTPUT_DIRECTORY when ava…

1fddcee

…ilable

feat(cmake): add sanitizer options for gcc

80d4611

fix(cmake): static builds

fd458de

refactor(cmake): add messages

1926d32

fix(format-project): shellcheck issues

36fa52d

FilipovicLado requested a review from tobre1 November 22, 2023 00:55

refactor(cmake/sanitizer): more explicit compiler id check

396c57e

Curve added 7 commits November 27, 2023 21:49

fix: update formatting

1b96dce

fix(workflows): build commands

1032eb6

fix(workflows(windows): long paths

e972ef2

fix(CMakeLists): disable VTK rendering group

74df39c

docs(readme): update build instructions

9c40d8a

docs(readme): fix test command

3ca2284

fix: ctest excludes

7757f39

refactor: disable sanitizer for msvc and apple clang

6b062d6

Curve added 3 commits November 29, 2023 12:43

fix(ci/windows): enable long paths

1184801

refactor(cmake): disable wrapping for vtk and disable libproj

d75cd9c

refactor(cmake): disable sanitizer by default

e6b56f7

Curve commented Nov 29, 2023

View reviewed changes

tobre1 mentioned this pull request Nov 29, 2023

Address sanitizer fails in tests #100

Open

fix(windows): only export symbols for viennals

8c894ba

Curve added 2 commits November 29, 2023 15:50

refactor(cmake): exclude dependencies from all target

a3898eb

refactor(cmake): use get-cpm

6b5bef8

Curve added 2 commits November 29, 2023 16:00

refactor(cmake): lower required vtk version

6dbe999

* This should also fix the windows workflows as we've switched to a commit that includes upstream!10542

fix: formatting

5af2536

Curve added 2 commits December 2, 2023 17:13

fix: setup vtk env for targets

5b3f309

fix: formatting

38771b5

Curve force-pushed the cmake-refactor branch from a0dcb02 to 38771b5 Compare December 2, 2023 16:17

Curve added 4 commits December 2, 2023 17:27

fix: remove debug print

357a02c

fix(cmakelists): vtk dir on linux

2aa3da1

fix: skip vtk environment on non windows platforms

c95fd8d

fix: formatting

a8acd4c

Curve force-pushed the cmake-refactor branch from 55f8a2c to aef0bbf Compare December 2, 2023 23:51

refactor: more logging, don't use early return in macro

0a440c9

* early return behavior in macro may vary in different cmake versions

Curve force-pushed the cmake-refactor branch from aef0bbf to 0a440c9 Compare December 2, 2023 23:53

refactor: adjust target naming

05eb427

Curve mentioned this pull request Dec 6, 2023

Update Documentation & Github Actions #101

Merged

tobre1 approved these changes Dec 7, 2023

View reviewed changes

tobre1 merged commit 1dda578 into ViennaTools:master Dec 7, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize CMake #99

Modernize CMake #99

Curve commented Nov 21, 2023 •

edited

Loading

Curve commented Nov 27, 2023

tobre1 commented Nov 27, 2023

Curve commented Nov 27, 2023 •

edited

Loading

tobre1 commented Nov 27, 2023

Curve commented Nov 27, 2023

Curve commented Nov 27, 2023 •

edited

Loading

tobre1 commented Nov 29, 2023

Curve commented Nov 29, 2023

Curve commented Nov 29, 2023 •

edited

Loading

Curve Nov 29, 2023

Curve commented Nov 29, 2023 •

edited

Loading

tobre1 commented Nov 29, 2023

Curve commented Nov 29, 2023

Curve commented Nov 29, 2023

Curve commented Nov 29, 2023 •

edited

Loading

		@@ -30,22 +30,24 @@ jobs:
		key: viennals-dependency-cache-${{ runner.os }}-${{env.BUILD_TYPE}}-${{ hashFiles( './external/upstream/**CMakeLists.txt' ) }}

Modernize CMake #99

Modernize CMake #99

Conversation

Curve commented Nov 21, 2023 • edited Loading

Features

To Discuss

Breaking / Important Changes

Things I've tested

Curve commented Nov 27, 2023

tobre1 commented Nov 27, 2023

Curve commented Nov 27, 2023 • edited Loading

tobre1 commented Nov 27, 2023

Curve commented Nov 27, 2023

Curve commented Nov 27, 2023 • edited Loading

tobre1 commented Nov 29, 2023

Curve commented Nov 29, 2023

Curve commented Nov 29, 2023 • edited Loading

Curve Nov 29, 2023

Choose a reason for hiding this comment

Curve commented Nov 29, 2023 • edited Loading

tobre1 commented Nov 29, 2023

Curve commented Nov 29, 2023

Curve commented Nov 29, 2023

Curve commented Nov 29, 2023 • edited Loading

Curve commented Nov 21, 2023 •

edited

Loading

Curve commented Nov 27, 2023 •

edited

Loading

Curve commented Nov 27, 2023 •

edited

Loading

Curve commented Nov 29, 2023 •

edited

Loading

Curve commented Nov 29, 2023 •

edited

Loading

Curve commented Nov 29, 2023 •

edited

Loading