github actions for building and testing - no commit history #8

doodeck · 2023-04-06T18:33:29Z

Added two github actions and corresponding badges in central README.md:
- Building the library without tests
- Building the library with tests (-DNF9_BUILD_TESTS=ON) and executing them. To make it possible, explicitly pulling external libraries libtins (as a git submodule) and google test (download GitHub ZIP archive)
Added README.md hints for building on MacOS M1
Collapsed the history of 23 commits using “Squash and Merge”. If interested you can still see in in doodeck/libnetflow9

Please review and consider for merging.

mkcms · 2023-04-12T07:01:07Z

.github/workflows/cmake.yml

+    - uses: actions/checkout@v3
+
+    - name: Configure CMake
+      # Configure CMake in a 'build' subdirectory. `CMAKE_BUILD_TYPE` is only required if you are using a single-configuration generator such as make.


Keep lines shorter, we try to keep a limit of 79 characters per line

these comments were provided by github.com, when creating the action file. we can remove them.

mkcms · 2023-04-12T07:05:20Z

CMakeLists.txt

@@ -29,7 +29,12 @@ else ()
  add_library(netflow9 STATIC ${CXX_SRC})
 endif ()

-target_include_directories(netflow9 SYSTEM PUBLIC include)
+get_property(SRC_DIR DIRECTORY PROPERTY SOURCE_DIR)
+message(STATUS "tins include:" ${SRC_DIR}/external/libtins/include/)


Looks like you left this line for debugging. It's pretty useless information - it does not depend on anything external, and will always be the same once you download the repository, there is no way to change it.

mkcms · 2023-04-12T07:05:52Z

README.md

+
+Apple clang version 14.0.0 (clang-1400.0.29.202)  
+
+is incapable of compiling the library.


Can you provide more details about this?

Are you able to see that build log:

https://github.com/doodeck/libnetflow9/actions/runs/4681045806/jobs/8293144058

?
CMake Error at CMakeLists.txt:75 (message):
Could not find required <memory_resource> or <experimental/memory_resource>
headers

I guess the library was designed and build on top of gcc toolset. What is causing problems here is probably CLang toolset. Actually they might have fixed it for the future releases:

https://reviews.llvm.org/D89057

mkcms · 2023-04-12T07:06:40Z

test/CMakeLists.txt

+message(STATUS "googletest populate: " ${googletest_SOURCE_DIR} " " ${googletest_BINARY_DIR})
+
+get_property(SRC_DIR DIRECTORY PROPERTY SOURCE_DIR)
+message(STATUS "tins libs: " ${SRC_DIR}/../external/libtins/build/lib/)


This is also pretty useless information, since the path never changes.

Yes, it can be removed.

mkcms · 2023-04-12T07:08:21Z

test/CMakeLists.txt


-find_library(LIBTINS_LIBRARIES tins)
+message(STATUS "googletest populate: " ${googletest_SOURCE_DIR} " " ${googletest_BINARY_DIR})


This message could be more meaningful. What does it mean?
Also, if I just want to build the library, I don't need to see this - instead of a STATUS message this could be DEBUG.

Debug message again, not needed anymore

mkcms · 2023-04-12T07:09:25Z

.github/workflows/tests.yml

+
+env:
+  # Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.)
+  BUILD_TYPE: Release


For tests we probably want to build in Debug mode with sanitizers etc. right?

Sure, and I've just checked, that the tests still work afterwards with Debug.

mkcms · 2023-04-12T07:12:14Z

test/CMakeLists.txt

+)
+# For Windows: Prevent overriding the parent project's compiler/linker settings
+set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
+FetchContent_MakeAvailable(googletest)


That's cool but I think there should be an option to use system provided GTest still.
Sometimes, projects that deal with networking/security (such as this one) are built in airgapped environments.

Sure, we can switch the behaviour on options. Actually, FetchContent_MakeAvailable is the approach recommended by Google (https://github.com/google/googletest/blob/main/googletest/README.md).

"Specify the commit you depend on and update it regularly." might be fine for GTest, in general we prefer to depend on distro (Debian in most cases) libraries that get some security team support (and yet are stable in version). Not everywhere it's possible (npm, python packages), but it's a pain to "update commits regularly".

For gtest I would not care, but still, it would be best if it worked internally without calling gh - internal mirror on bitbucket in this case. So URL would have to be a variable (?). And then we have a mirror to update too. We try to care about the delivery chain.

mkcms · 2023-04-12T07:14:16Z

.gitmodules

@@ -0,0 +1,3 @@
+[submodule "external/libtins"]


Is it really better than using a system library?
Also, is a submodule better than, e.g. a subtree? Personally I always found git-submodules difficult to use.

System library is good, provided it is there. It happens to be the case on Ubuntu - github runner, so we can switch back to it there. On RedHat/CentOS/Fedora derivatives it may not be the case. I tried it on AWS Linux 2 (ID_LIKE="centos rhel fedora") and Oracle Linux (fedora) and needed to compile libtins myself, hence referencing it as a submodule. You may need to get used to using submodules, but they are based on healthy principles. You keep just a reference to the dependant repository, so there is no code duplication. Actually, libtins library itself is using googletest as a submodule https://github.com/mfontanini/libtins/blob/master/.gitmodules .

It should still work in airgapped environment or via an internal cache (source of truth for this repository up to this PR was internal company Bitbucket. There's sometimes limited access to gh too. It's nice idea to have it build easily on RH-like distros, we'd just need to make certain it still works in current envs - and/or review policies.

Also, probably this should reference a specific tag/commit which is known to work?

Create cmake.yml Add MacOS builiding hints Add libtins as a submodule Add build hints for external libtins and gtest Create tests.yml Upgrade google test version 1.11.0 -> 1.13.0 Add build and tests badges to README Address the issues in the discussion of the PR: * use the system libraries, make alternatives optional, activated with switches * keep the lines up to 80 chars in length, unless technically not possible * set the level of redundant messages to DEBUG, mark them for removal with FIXME: * provide details of build problems using Apple/CLang in README * Use built type Debug for tests

Not needed in master, is not operational yet, used only to reproduce MacOS problems

doodeck · 2023-04-23T15:08:18Z

Added two github actions and corresponding badges in central README.md:

Building the library without tests

Building the library with tests (-DNF9_BUILD_TESTS=ON) and executing them. To make it possible, explicitly pulling external libraries libtins (as a git submodule) and google test (download GitHub ZIP archive)

Added README.md hints for building on MacOS M1

Collapsed the history of 23 commits using “Squash and Merge”. If interested you can still see in in doodeck/libnetflow9

Please review and consider for merging.

I've addressed all the remarks in the discussion to PR.
All the changes are mentioned in the comment to the commit e950390.

mskwarek · 2023-05-23T19:50:17Z

README.md

+
+![test workflow sys](https://github.com/doodeck/libnetflow9/actions/workflows/tests.yml/badge.svg)
+
+![test workflow own](https://github.com/doodeck/libnetflow9/actions/workflows/tests-own.yml/badge.svg)


Shouldn't those links link to the exatel/libnetflow9 repo?

Excellent point! Ultimately - they should. As long as the changes exist only in a pull-request we will probably see an empty status. This is a chicken - egg problem, I guess, I should change it right now.

mskwarek · 2023-05-23T19:54:58Z

.github/workflows/tests.yml

+      run: sudo apt-get install -y libgtest-dev
+
+    - name: Install libtins
+      run: sudo apt-get install -y libtins-dev


We probably may install all dependencies in one step:
sudo apt-get install -y libpcap-dev libgtest-dev libtins-dev

mskwarek · 2023-05-23T20:01:37Z

.github/workflows/tests-own.yml

+    - name: Build libtins library
+      run: |
+        cd ${{github.workspace}}/external/libtins && mkdir build && \
+          cd build && cmake ..  -DLIBTINS_ENABLE_CXX11=1 && cmake --build .


Marking submodule to fixed commit would be nice IMHO. It might be a little more painful to switch any external library to newer version but won't cause pipeline failures when something go wrong in dependencies.

Anyway I don't get how own version of googletest is installed - git submodule update handles it? It's a bit implicit for me tbh.

…roject For the time being, in pull request's branch, the badges are expected to remain grey and show "no status"

mkcms reviewed Apr 12, 2023

View reviewed changes

doodeck force-pushed the feature_branch_no_history branch from c0ca159 to e950390 Compare April 23, 2023 14:53

Delete cmake-macos.yml

1fe7671

Not needed in master, is not operational yet, used only to reproduce MacOS problems

mskwarek reviewed May 23, 2023

View reviewed changes

Make build/test badges point to the (future) actions in forked-from p…

7a10444

…roject For the time being, in pull request's branch, the badges are expected to remain grey and show "no status"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

github actions for building and testing - no commit history #8

github actions for building and testing - no commit history #8

doodeck commented Apr 6, 2023

mkcms Apr 12, 2023

doodeck Apr 12, 2023 •

edited

Loading

mkcms Apr 12, 2023

mkcms Apr 12, 2023

doodeck Apr 12, 2023

mkcms Apr 12, 2023

doodeck Apr 12, 2023

mkcms Apr 12, 2023

doodeck Apr 12, 2023

mkcms Apr 12, 2023

doodeck Apr 12, 2023

mkcms Apr 12, 2023

doodeck Apr 12, 2023

blaa Apr 12, 2023

mkcms Apr 12, 2023

doodeck Apr 12, 2023

blaa Apr 12, 2023

doodeck commented Apr 23, 2023 •

edited

Loading

mskwarek May 23, 2023

doodeck Jun 15, 2023

mskwarek May 23, 2023

mskwarek May 23, 2023


		Apple clang version 14.0.0 (clang-1400.0.29.202)

		is incapable of compiling the library.


		find_library(LIBTINS_LIBRARIES tins)
		message(STATUS "googletest populate: " ${googletest_SOURCE_DIR} " " ${googletest_BINARY_DIR})


		![test workflow sys](https://github.com/doodeck/libnetflow9/actions/workflows/tests.yml/badge.svg)

		![test workflow own](https://github.com/doodeck/libnetflow9/actions/workflows/tests-own.yml/badge.svg)

github actions for building and testing - no commit history #8

Are you sure you want to change the base?

github actions for building and testing - no commit history #8

Conversation

doodeck commented Apr 6, 2023

Choose a reason for hiding this comment

doodeck Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doodeck commented Apr 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doodeck Apr 12, 2023 •

edited

Loading

doodeck commented Apr 23, 2023 •

edited

Loading