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

contrib: Shift vpp build -> rules_foreign_cc #27243

Merged
merged 2 commits into from
May 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions bazel/envoy_build_system.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,17 @@ def envoy_cmake(
name,
cache_entries = {},
debug_cache_entries = {},
default_cache_entries = {"CMAKE_BUILD_TYPE": "Bazel"},
lib_source = "",
postfix_script = "",
copy_pdb = False,
pdb_name = "",
cmake_files_dir = "$BUILD_TMPDIR/CMakeFiles",
generate_crosstool_file = False,
generate_args = ["-GNinja"],
targets = ["", "install"],
**kwargs):
cache_entries.update({"CMAKE_BUILD_TYPE": "Bazel"})
Copy link
Member Author

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

Copy link
Member

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

cache_entries.update(default_cache_entries)
cache_entries_debug = dict(cache_entries)
cache_entries_debug.update(debug_cache_entries)

Expand Down Expand Up @@ -152,8 +155,8 @@ def envoy_cmake(
"@envoy//bazel:dbg_build": cache_entries_debug,
"//conditions:default": cache_entries,
}),
generate_args = ["-GNinja"],
targets = ["", "install"],
generate_args = generate_args,
targets = targets,
# TODO: Remove install target and make this work
install = False,
# TODO(lizan): Make this always true
Expand Down
158 changes: 152 additions & 6 deletions bazel/foreign_cc/vpp_vcl.patch
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)
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member

@florincoras florincoras May 7, 2023

Choose a reason for hiding this comment

The 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 vpp_vcl_build.sh:

cmake -G Ninja ../src -DCMAKE_BUILD_TYPE:STRING=release
ninja -C . vppcom

In other words, we're only building vppcom lib target and its dependencies, not the whole of vpp. The heavy part is vnet not the binary api. Did the build_args in envoy_cmake lower not work?

Copy link
Member Author

@phlax phlax May 7, 2023

Choose a reason for hiding this comment

The 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 @@
Expand All @@ -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)
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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
2 changes: 1 addition & 1 deletion bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -1317,7 +1317,7 @@ filegroup(
def _com_github_fdio_vpp_vcl():
external_http_archive(
name = "com_github_fdio_vpp_vcl",
build_file_content = BUILD_ALL_CONTENT,
build_file_content = _build_all_content(exclude = ["**/*doc*/**", "**/examples/**", "**/plugins/**"]),
patches = ["@envoy//bazel/foreign_cc:vpp_vcl.patch"],
)

Expand Down
73 changes: 50 additions & 23 deletions contrib/vcl/source/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"],
Expand All @@ -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(
Copy link
Member

@florincoras florincoras May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we maybe leverage something like this first approach to building vppcom? Note that the file eventually had some additional changes but was finally removed here in favor of the approach we're now deprecating.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 make - there was no equivalent setup for cmake anyway)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting a hermetic cmake toolchain, which i think is not trivial with foreign_cc

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(
Expand Down
29 changes: 0 additions & 29 deletions contrib/vcl/source/vcl_build_launcher.py

This file was deleted.

Loading