cmake: modify build system for OpenWrt compatibility #172

liudf0716 · 2024-11-14T09:44:13Z

Disable testing and documentation builds by default
- Add conditional inclusion of tests and documentation
- Handle argp dependency with find_library
- Make development tools (clang-tidy, clang-format) optional
- Add flexible bpftool handling with fallback paths
- Improve build system modularity and configurability
This modification adjusts the build system to ensure compatibility
with the OpenWrt environment, making necessary changes for a successful build.

Signed-off-by: Dengfeng Liu liudf0716@gmail.com

- Disable testing and documentation builds by default - Add conditional inclusion of tests and documentation - Handle argp dependency with find_library - Make development tools (clang-tidy, clang-format) optional - Add flexible bpftool handling with fallback paths - Improve build system modularity and configurability This modification adjusts the build system to ensure compatibility with the OpenWrt environment, making necessary changes for a successful build. Signed-off-by: Dengfeng Liu <liudf0716@gmail.com>

Signed-off-by: Dengfeng Liu <liudf0716@gmail.com>

facebook-github-bot · 2024-11-14T09:44:19Z

Hi @liudf0716!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

facebook-github-bot · 2024-11-14T10:03:47Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

qdeslandes · 2024-11-25T14:06:25Z

CMakeLists.txt

+option(BUILD_TESTING "Enable testing" OFF)
+option(BUILD_DOCS "Enable building documentation" OFF)


Those should be enabled by default to keep the current behaviour. Use WITH_XXX instead of BUILD_XXX similarly to #174.

qdeslandes · 2024-11-25T14:08:29Z

CMakeLists.txt

+option(BUILD_DOCS "Enable building documentation" OFF)
+
+if(BUILD_DOCS)
+    find_package(Doxygen REQUIRED)


Move this in docs/CMakeLists.txt, and modify add_subdirectory(docs) below to:

if (WITH_DOCS) add_subdirectory(docs) endif ()

qdeslandes · 2024-11-25T14:08:42Z

CMakeLists.txt

+if(BUILD_TESTING)
+    pkg_check_modules(cmocka REQUIRED IMPORTED_TARGET cmocka)
+endif()


qdeslandes · 2024-11-25T14:10:43Z

CMakeLists.txt

 pkg_check_modules(nl REQUIRED IMPORTED_TARGET libnl-3.0)

+find_library(ARGP_LIBRARY 
+    NAMES argp
+    PATHS ${CMAKE_SYSROOT}/usr/lib 


According to CMake's doc, search path are automatically prefixed with CMAKE_SYSROOT, so there is no need to add it here.

qdeslandes · 2024-11-25T14:12:13Z

CMakeLists.txt

+if(NOT ARGP_LIBRARY)
+    message(FATAL_ERROR "libargp not found")
+endif()


ARGP_LIBRARY is REQUIRED above, if not found, CMake will fail the generation and print an error message, no need for this condition.

qdeslandes · 2024-11-25T14:12:30Z

CMakeLists.txt

+if(BUILD_DOCS)
+    find_program(SPHINX_BIN sphinx-build REQUIRED)
+    find_program(LCOV_BIN lcov REQUIRED)
+    find_program(GENHTML_BIN genhtml REQUIRED)
+    find_program(CLANG_TIDY_BIN NAMES clang-tidy-18 clang-tidy REQUIRED)
+    find_program(CLANG_FORMAT_BIN NAMES clang-format-18 clang-format REQUIRED)
+endif()


All those find_program should be located in docs/CMakeLists.txt.

qdeslandes · 2024-11-25T14:13:46Z

src/core/helper.h

@@ -80,7 +78,7 @@ extern const char *strerrordesc_np(int errnum);
 *
 * @param v Error code, can be positive or negative.
 */
-#define bf_strerror(v) strerrordesc_np(abs(v))
+#define bf_strerror(v) strerror(abs(v))


Why this change?

qdeslandes · 2024-11-25T14:15:16Z

CMakeLists.txt

+    find_program(CLANG_TIDY_BIN NAMES clang-tidy-18 clang-tidy REQUIRED)
+    find_program(CLANG_FORMAT_BIN NAMES clang-format-18 clang-format REQUIRED)


Those two binaries are not part of the documentation generation, but the testing logic.

qdeslandes · 2024-11-25T14:21:04Z

CMakeLists.txt

+# Replace mandatory REQUIRED flag
+find_program(BPFTOOL_BIN bpftool)
+
+# Add fallback paths
+if(NOT BPFTOOL_BIN)
+    find_program(BPFTOOL_BIN bpftool
+        PATHS
+        ${CMAKE_SYSROOT}/usr/sbin
+        ${CMAKE_SYSROOT}/sbin
+        /usr/sbin
+        /sbin
+    )
+endif()
+
+# Make it optional with warning
+if(NOT BPFTOOL_BIN)
+    message(WARNING "bpftool not found - some features may be disabled")
+    set(BPFTOOL_BIN "bpftool") # Set default value
+endif()


Move this to tests/bpf/CMakeLists.txt, where bpftool is actually used. It's not ideal, but better than the current situation and probably sufficient for your use case.

qdeslandes · 2024-11-25T14:26:45Z

Thanks for the PR @liudf0716 ! I've recently modified the benchmarking configuration similarly to this PR, see #174 for a example of what I'm looking for. Long story short:

Dependencies of specific parts of the projects (e.g. doc, tests) should be discovered within the related CMakeLists.txt.
Options are defined in the top-level CMakeLists.txt to enable/disable those different parts (e.g. -DWITH_TESTS).
If a specific option is defined, the related directory should be processed by CMake.
The new option's default value should be set to maintain the current behaviour (e.g. WITH_TESTS should default to ON as tests are always enabled at the moment).

qdeslandes

Updating status

liudf0716 added 2 commits November 14, 2024 17:42

helper: replace strerrordesc_np with strerror for error handling

f7c62f2

Signed-off-by: Dengfeng Liu <liudf0716@gmail.com>

liudf0716 requested a review from qdeslandes as a code owner November 14, 2024 09:44

liudf0716 mentioned this pull request Nov 14, 2024

does bpfilter support redirect? #167

Open

facebook-github-bot added the cla signed label Nov 14, 2024

qdeslandes reviewed Nov 25, 2024

View reviewed changes

qdeslandes self-requested a review November 25, 2024 15:25

qdeslandes requested changes Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: modify build system for OpenWrt compatibility #172

cmake: modify build system for OpenWrt compatibility #172

liudf0716 commented Nov 14, 2024

facebook-github-bot commented Nov 14, 2024

facebook-github-bot commented Nov 14, 2024

qdeslandes Nov 25, 2024

qdeslandes Nov 25, 2024

qdeslandes Nov 25, 2024

qdeslandes Nov 25, 2024

qdeslandes Nov 25, 2024

qdeslandes Nov 25, 2024

qdeslandes Nov 25, 2024

qdeslandes Nov 25, 2024

qdeslandes Nov 25, 2024

qdeslandes commented Nov 25, 2024

qdeslandes left a comment

		option(BUILD_TESTING "Enable testing" OFF)
		option(BUILD_DOCS "Enable building documentation" OFF)

		find_program(CLANG_TIDY_BIN NAMES clang-tidy-18 clang-tidy REQUIRED)
		find_program(CLANG_FORMAT_BIN NAMES clang-format-18 clang-format REQUIRED)

cmake: modify build system for OpenWrt compatibility #172

Are you sure you want to change the base?

cmake: modify build system for OpenWrt compatibility #172

Conversation

liudf0716 commented Nov 14, 2024

facebook-github-bot commented Nov 14, 2024

Action Required

Process

facebook-github-bot commented Nov 14, 2024

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

qdeslandes commented Nov 25, 2024

qdeslandes left a comment

Choose a reason for hiding this comment