-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
contrib: Shift vpp
build -> rules_foreign_cc
#27243
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
# Not a git repo so embed version | ||
diff --git src/CMakeLists.txt src/CMakeLists.txt | ||
index 4be247333..230c667ff 100644 | ||
--- src/CMakeLists.txt | ||
+++ src/CMakeLists.txt | ||
@@ -40,12 +40,8 @@ include(cmake/ccache.cmake) | ||
|
@@ -16,16 +17,18 @@ | |
string(REPLACE "-" ";" VPP_LIB_VERSION ${VPP_VERSION}) | ||
list(GET VPP_LIB_VERSION 0 VPP_LIB_VERSION) | ||
|
||
@@ -188,7 +184,7 @@ elseif("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") | ||
@@ -188,8 +184,7 @@ elseif("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") | ||
find_package(OpenSSL) | ||
set(SUBDIRS | ||
vppinfra svm vlib vlibmemory vlibapi vnet vpp vat vat2 vcl vpp-api | ||
- plugins tools/vppapigen tools/g2 tools/perftool cmake pkg | ||
+ tools/vppapigen tools/g2 tools/perftool cmake pkg | ||
tools/appimage | ||
- tools/appimage | ||
+ tools/vppapigen cmake pkg | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. im guessing a lot of this is not required, but i guess also that with the right flags/target it would be ignored There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Components in vpp are unfortunately not as cleanly separated as we'd want. Still, until now we could do with a minimal build because of this snippet in
In other words, we're only building There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure what should/not build tbh it was my first real experience of cmake and i now understand why its so disliked i tried so many different ways, but cmake's use of anything i set was patchy at best and a long way from the scant docs i could find ideally we update the cmake config in such a way as we can cleanly call just the target we actually need if its difficult to do there, its an order of manitude more difficult doing it with patches etc |
||
) | ||
elseif("${CMAKE_SYSTEM_NAME}" STREQUAL "Darwin") | ||
|
||
set(SUBDIRS vppinfra) | ||
diff --git src/cmake/ccache.cmake src/cmake/ccache.cmake | ||
index 058a0f3d8..30dcb0c15 100644 | ||
--- src/cmake/ccache.cmake | ||
+++ src/cmake/ccache.cmake | ||
@@ -14,7 +14,7 @@ | ||
|
@@ -37,7 +40,8 @@ | |
if(VPP_USE_CCACHE) | ||
find_program(CCACHE_FOUND ccache) | ||
message(STATUS "Looking for ccache") | ||
|
||
diff --git src/cmake/library.cmake src/cmake/library.cmake | ||
index ad4adfcab..0051bca10 100644 | ||
--- src/cmake/library.cmake | ||
+++ src/cmake/library.cmake | ||
@@ -24,7 +24,7 @@ macro(add_vpp_library lib) | ||
|
@@ -49,3 +53,145 @@ | |
target_sources(${lib} PRIVATE $<TARGET_OBJECTS:${lo}>) | ||
|
||
if(VPP_LIB_VERSION) | ||
diff --git src/tools/vppapigen/vppapigen.py src/tools/vppapigen/vppapigen.py | ||
index 8415c28fb..a017d9fc6 100755 | ||
--- src/tools/vppapigen/vppapigen.py | ||
+++ src/tools/vppapigen/vppapigen.py | ||
@@ -7,6 +7,13 @@ import logging | ||
import binascii | ||
import os | ||
from subprocess import Popen, PIPE | ||
+ | ||
+# Put ply on the path ... | ||
+plypath = os.path.join( | ||
+ os.environ["EXT_BUILD_ROOT"], | ||
+ os.path.dirname(os.environ["PLYPATHS"].split()[0])) | ||
+sys.path += [plypath] | ||
+ | ||
import ply.lex as lex | ||
import ply.yacc as yacc | ||
|
||
diff --git src/vat/CMakeLists.txt src/vat/CMakeLists.txt | ||
index e5945b20d..3d76f3d88 100644 | ||
--- src/vat/CMakeLists.txt | ||
+++ src/vat/CMakeLists.txt | ||
@@ -19,39 +19,6 @@ add_vpp_library(vatplugin | ||
LINK_LIBRARIES vppinfra | ||
) | ||
|
||
-############################################################################## | ||
-# vpp_api_test | ||
-############################################################################## | ||
-add_vpp_executable(vpp_api_test ENABLE_EXPORTS | ||
- SOURCES | ||
- api_format.c | ||
- main.c | ||
- plugin.c | ||
- json_format.c | ||
- types.c | ||
- ip_types_api.c | ||
- ip_types.c | ||
- protocols.def | ||
- | ||
- DEPENDS api_headers | ||
- | ||
- LINK_LIBRARIES | ||
- vlibmemoryclient | ||
- svm | ||
- vatplugin | ||
- vppinfra | ||
- Threads::Threads | ||
- dl | ||
-) | ||
- | ||
-############################################################################## | ||
-# vpp_json_test | ||
-############################################################################## | ||
-add_vpp_executable(vpp_json_test ENABLE_EXPORTS NO_INSTALL | ||
- SOURCES json_format.c json_test.c | ||
- LINK_LIBRARIES vppinfra m | ||
-) | ||
- | ||
############################################################################## | ||
# vat headers | ||
############################################################################## | ||
diff --git src/vat2/CMakeLists.txt src/vat2/CMakeLists.txt | ||
index 108e184b5..a90107617 100644 | ||
--- src/vat2/CMakeLists.txt | ||
+++ src/vat2/CMakeLists.txt | ||
@@ -14,21 +14,6 @@ | ||
############################################################################## | ||
# vat2 | ||
############################################################################## | ||
-add_vpp_executable(vat2 ENABLE_EXPORTS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i stripped out quite a bit here, possibly its not needed and was only building due to incorrect cmake flags, not sure either way it was throwing errors about duplicate symbols |
||
- SOURCES | ||
- main.c | ||
- plugin.c | ||
- | ||
- DEPENDS api_headers | ||
- | ||
- LINK_LIBRARIES | ||
- vlibmemoryclient | ||
- svm | ||
- vppinfra | ||
- vppapiclient | ||
- Threads::Threads | ||
- dl | ||
-) | ||
|
||
# | ||
# Unit test code. Call generator directly to avoid it being installed | ||
@@ -37,20 +22,6 @@ add_vpp_executable(vat2 ENABLE_EXPORTS | ||
#) | ||
|
||
vpp_generate_api_c_header (test/vat2_test.api) | ||
-add_vpp_executable(test_vat2 ENABLE_EXPORTS NO_INSTALL | ||
- SOURCES | ||
- test/vat2_test.c | ||
- | ||
- DEPENDS api_headers | ||
- | ||
- LINK_LIBRARIES | ||
- vlibmemoryclient | ||
- svm | ||
- vppinfra | ||
- vppapiclient | ||
- Threads::Threads | ||
- dl | ||
-) | ||
#target_link_options(test_vat2 PUBLIC "LINKER:-fsanitize=address") | ||
if(VPP_BUILD_TESTS_WITH_COVERAGE) | ||
set(TARGET_NAME test_vat2) | ||
diff --git src/vpp-api/CMakeLists.txt src/vpp-api/CMakeLists.txt | ||
index 72cc1b29c..0f2510d51 100644 | ||
--- src/vpp-api/CMakeLists.txt | ||
+++ src/vpp-api/CMakeLists.txt | ||
@@ -29,10 +29,5 @@ add_vpp_headers(vpp-api | ||
client/stat_client.h | ||
) | ||
|
||
-add_vpp_executable(test_vppapiclient NO_INSTALL | ||
- SOURCES client/test.c | ||
- LINK_LIBRARIES vppinfra pthread vppapiclient | ||
-) | ||
- | ||
add_subdirectory(vapi) | ||
add_subdirectory(python) | ||
diff --git src/vpp/CMakeLists.txt src/vpp/CMakeLists.txt | ||
index 1b43b1299..361b981d9 100644 | ||
--- src/vpp/CMakeLists.txt | ||
+++ src/vpp/CMakeLists.txt | ||
@@ -83,13 +83,6 @@ if(VPP_API_TEST_BUILTIN) | ||
add_definitions(-DVPP_API_TEST_BUILTIN=1) | ||
endif() | ||
|
||
-add_vpp_executable(vpp | ||
- ENABLE_EXPORTS | ||
- SOURCES ${VPP_SOURCES} | ||
- LINK_LIBRARIES svm vlib vppinfra vlibmemory vnet Threads::Threads ${CMAKE_DL_LIBS} | ||
- DEPENDS vpp_version_h api_headers | ||
-) | ||
- | ||
add_vpp_headers(vpp | ||
stats/stat_segment.h | ||
stats/stat_segment_shared.h |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,9 @@ load( | |
"//bazel:envoy_build_system.bzl", | ||
"envoy_cc_contrib_extension", | ||
"envoy_cc_library", | ||
"envoy_cmake", | ||
"envoy_contrib_package", | ||
) | ||
load("@rules_python//python:defs.bzl", "py_binary") | ||
load("@base_pip3//:requirements.bzl", "requirement") | ||
|
||
licenses(["notice"]) # Apache 2 | ||
|
@@ -20,6 +20,7 @@ cc_library( | |
"external/libvlibmemoryclient.a", | ||
"external/libvppcom.a", | ||
"external/libvppinfra.a", | ||
"external/vppcom.h", | ||
], | ||
hdrs = ["external/vppcom.h"], | ||
defines = ["VPP_VCL"], | ||
|
@@ -28,37 +29,63 @@ cc_library( | |
visibility = ["//visibility:public"], | ||
) | ||
|
||
genrule( | ||
envoy_cmake( | ||
name = "build", | ||
srcs = [ | ||
"@com_github_fdio_vpp_vcl//:all", | ||
build_data = [requirement("ply")], | ||
cache_entries = { | ||
"CMAKE_BUILD_TYPE": "Release", | ||
"VPP_API_TEST_BUILTIN": "OFF", | ||
"BUILD_SHARED_LIBS": "OFF", | ||
"CMAKE_ENABLE_TESTING": "OFF", | ||
"CMAKE_ENABLE_EXPORTS": "OFF", | ||
}, | ||
# build_args = ["--", "-C", ".", "vppcom"], | ||
phlax marked this conversation as resolved.
Show resolved
Hide resolved
|
||
copts = ["-Wno-unused-variable"], | ||
default_cache_entries = {}, | ||
env = { | ||
"PLYPATHS": "$(locations %s)" % requirement("ply"), | ||
}, | ||
lib_source = "@com_github_fdio_vpp_vcl//:all", | ||
linkopts = ["-Wno-unused-variable"], | ||
out_static_libs = [ | ||
"libvppcom.a", | ||
"libvppinfra.a", | ||
"libsvm.a", | ||
"libvlibmemoryclient.a", | ||
], | ||
postfix_script = """ | ||
mkdir -p $INSTALLDIR/lib/external $INSTALLDIR/include/external \ | ||
&& find . -name "*.a" | xargs -I{} cp -a {} $INSTALLDIR/lib/ \ | ||
&& find . -name "*.h" | xargs -I{} cp -a {} $INSTALLDIR/include | ||
""", | ||
tags = [ | ||
"cpu:16", | ||
"skip_on_windows", | ||
], | ||
# tags = ["no-remote", "skip_on_windows"], | ||
phlax marked this conversation as resolved.
Show resolved
Hide resolved
|
||
targets = [ | ||
"", | ||
"install", | ||
], | ||
working_directory = "src", | ||
) | ||
|
||
genrule( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure which bit of that approach you mean, but that is the issue ideally we do this with rules_foreign_cc - doing it with a genrule requires getting a hermetic cmake toolchain, which i think is not trivial with foreign_cc (not sure why the documented example doesnt seem to work, and it was for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this should be easy - ive certainly found it so with other toolchains that ive worked with possibly we could do something similar to how we fish out the zstd bin here https://github.com/envoyproxy/envoy/blob/main/tools/zstd/zstd.bzl - altho its a little different as cmake/ninja are provided as a toolchain rather than just a tool tbh tho its just fighting the system - the better, easier to maintain way would be to get the cmake <> foreign_cc setup correct i think |
||
name = "build_files", | ||
outs = [ | ||
"external/libvppcom.a", | ||
"external/libvppinfra.a", | ||
"external/libsvm.a", | ||
"external/libvlibmemoryclient.a", | ||
"external/libvppcom.a", | ||
"external/libvppinfra.a", | ||
"external/vppcom.h", | ||
], | ||
cmd = """ | ||
./$(location :vcl_build_launcher) vpp_vcl_build.sh $(location external/libvppcom.a) | ||
EXTERNAL_DIR=$$(dirname $(location external/libsvm.a)) \ | ||
&& mkdir -p $$EXTERNAL_DIR \ | ||
&& find . -name "*.a" | xargs -I{} cp -a {} $$EXTERNAL_DIR \ | ||
&& find . -name "vppcom.h" | xargs -I{} cp -a {} $$EXTERNAL_DIR | ||
""", | ||
target_compatible_with = [ | ||
"@platforms//os:linux", | ||
], | ||
tools = [ | ||
":vcl_build_launcher", | ||
], | ||
) | ||
|
||
py_binary( | ||
name = "vcl_build_launcher", | ||
srcs = ["vcl_build_launcher.py"], | ||
data = [ | ||
"vpp_vcl_build.sh", | ||
], | ||
main = "vcl_build_launcher.py", | ||
deps = [requirement("ply")], | ||
exec_tools = [":build"], | ||
) | ||
|
||
envoy_cc_library( | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this is enforced - i think in the case of vpp we want it to be
Release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd have to dig up the history, there was a reason