contrib: Shift `vpp` build -> `rules_foreign_cc` #27243

phlax · 2023-05-07T09:31:56Z

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2023-05-07T09:32:04Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #27243 was opened by phlax.

see: more, trace.

phlax · 2023-05-07T09:34:32Z

cc @florincoras

i think this builds a lot unnecessarily but does kinda work (at least testing locally)

i tried setting the target to "" in that case it builds everything apart from the vppcom.h

ive tried to find a way of triggering ninja after as was done in the original build, but struggling to find a way to re/use the binary provided by rules_foreign_cc (cc @keith )

contrib/vcl/source/BUILD

phlax · 2023-05-07T09:37:23Z

contrib/vcl/source/BUILD

+    targets = ["", "install"],
+    build_data = [requirement("ply")],
+    env = {
+        "PLYPATHS": "$(locations %s)" % requirement("ply"),


there is probably a more elegant way to do this - but i was struggling to think of one

bazel/repositories.bzl

phlax · 2023-05-07T09:39:08Z

bazel/foreign_cc/vpp_vcl.patch

+ ##############################################################################
+ # vat2
+ ##############################################################################
+-add_vpp_executable(vat2 ENABLE_EXPORTS


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

phlax · 2023-05-07T09:39:50Z

bazel/foreign_cc/vpp_vcl.patch

-+    tools/vppapigen tools/g2 tools/perftool cmake pkg
-     tools/appimage
+-    tools/appimage
+    tools/vppapigen cmake pkg


im guessing a lot of this is not required, but i guess also that with the right flags/target it would be ignored

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?

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

phlax · 2023-05-07T09:40:26Z

bazel/envoy_build_system.bzl

        **kwargs):
-    cache_entries.update({"CMAKE_BUILD_TYPE": "Bazel"})


not sure why this is enforced - i think in the case of vpp we want it to be Release

i'd have to dig up the history, there was a reason

phlax · 2023-05-07T09:54:33Z

interesting, i now understand why the arm build can be much faster than x64

when this build (and probably same with other foreign builds) gets triggered, and the job is sent to RBE it gets built with 2 cores, whereas on arm its building locally with much more cores and everything on tmpfs

probably for these foreign builds we want to make it no-remote-exec but only when its a larger machine

phlax · 2023-05-07T10:13:17Z

contrib/vcl/source/BUILD

+        && find . -name "*.a" | xargs -I{} cp -a {} $INSTALLDIR/lib/ \
+        && find . -name "*.h" | xargs -I{} cp -a {} $INSTALLDIR/include
+    """,
+    tags = ["skip_on_windows", "cpu:16"],


not sure if this cpu setting does anything

Signed-off-by: Ryan Northey <ryan@synca.io>

florincoras · 2023-05-07T21:11:18Z

contrib/vcl/source/BUILD

+    working_directory = "src",
+)
+
+genrule(


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.

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)

for ref the make example

https://github.com/bazelbuild/rules_foreign_cc/blob/7fa1a1259bbc4fbceb4dc3a23ed520907b5d8d1d/examples/current_toolchains/BUILD.bazel#L1-L13

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

RyanTheOptimist

I don't really understand what's going on in this PR, though I'm sure you do :) Is there another maintainer who might? If so, I think it would be good to have them review. If not, I can stamp it, but if someone else is knowledgable, that'd be great!

phlax · 2023-05-08T20:44:15Z

experimentiing further with this locally and discussing offline with @florincoras - my take is that this is probably not a bad approximation to what we have now

i have tried removing the install step - so we are only running cmake with the default target (and with a lot of hacking to remove stuff) but it doesnt seem to decrease the compile time (ie the cmake step) much/any

running without install we dont get the header produced, but with it we do - this is essentially what was being created by running the vppcom build in the previous way

despite various attempts we didnt manage to get the patch to produce the header as part of a default (build?) run

keith · 2023-05-08T22:00:26Z

bazel/envoy_build_system.bzl

        **kwargs):
-    cache_entries.update({"CMAKE_BUILD_TYPE": "Bazel"})


i'd have to dig up the history, there was a reason

contrib/vcl/source/BUILD

Signed-off-by: Ryan Northey <ryan@synca.io>

florincoras

LGTM

Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: phlax <phlax@users.noreply.github.com>

Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 7, 2023

repokitteh-read-only bot assigned RyanTheOptimist May 7, 2023

phlax changed the title ~~contrib: Shift vpp build -> rules_foreign_cc~~ [WIP] contrib: Shift vpp build -> rules_foreign_cc May 7, 2023

phlax marked this pull request as draft May 7, 2023 09:32

phlax commented May 7, 2023

View reviewed changes

phlax force-pushed the foreign-vpp branch from 1722566 to 33c1463 Compare May 7, 2023 10:12

phlax commented May 7, 2023

View reviewed changes

phlax force-pushed the foreign-vpp branch from 33c1463 to 5f7f973 Compare May 7, 2023 10:24

contrib: Shift vpp build -> rules_foreign_cc

38748b7

Signed-off-by: Ryan Northey <ryan@synca.io>

phlax force-pushed the foreign-vpp branch from 5f7f973 to 38748b7 Compare May 7, 2023 10:45

phlax changed the title ~~[WIP] contrib: Shift vpp build -> rules_foreign_cc~~ contrib: Shift vpp build -> rules_foreign_cc May 7, 2023

phlax marked this pull request as ready for review May 7, 2023 10:45

phlax mentioned this pull request May 7, 2023

tracing: Transition to OpenTelemetry from OpenTracing and OpenCensus #9958

Open

florincoras reviewed May 7, 2023

View reviewed changes

RyanTheOptimist reviewed May 8, 2023

View reviewed changes

phlax assigned keith and florincoras and unassigned RyanTheOptimist May 8, 2023

phlax mentioned this pull request May 8, 2023

Reserve more CPU cores for integration tests with high rate of flakes #27257

Merged

keith previously approved these changes May 8, 2023

View reviewed changes

pr-feedback

bc60d52

Signed-off-by: Ryan Northey <ryan@synca.io>

phlax dismissed keith’s stale review via bc60d52 May 8, 2023 22:11

florincoras approved these changes May 8, 2023

View reviewed changes

phlax enabled auto-merge (squash) May 8, 2023 22:20

keith approved these changes May 8, 2023

View reviewed changes

phlax merged commit b896b6d into envoyproxy:main May 8, 2023

phlax added a commit to phlax/envoy that referenced this pull request Jun 1, 2023

contrib: Shift vpp build -> rules_foreign_cc (envoyproxy#27243)

c06de1d

Signed-off-by: Ryan Northey <ryan@synca.io>

phlax added a commit to phlax/envoy that referenced this pull request Jun 1, 2023

contrib: Shift vpp build -> rules_foreign_cc (envoyproxy#27243)

7f25a4f

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: phlax <phlax@users.noreply.github.com>

phlax added a commit to phlax/envoy that referenced this pull request Jun 1, 2023

contrib: Shift vpp build -> rules_foreign_cc (envoyproxy#27243)

553962b

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: phlax <phlax@users.noreply.github.com>

phlax added a commit to phlax/envoy that referenced this pull request Jun 1, 2023

contrib: Shift vpp build -> rules_foreign_cc (envoyproxy#27243)

d597759

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: phlax <phlax@users.noreply.github.com>

phlax added a commit that referenced this pull request Jun 2, 2023

contrib: Shift vpp build -> rules_foreign_cc (#27243)

06504d1

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: phlax <phlax@users.noreply.github.com>

phlax added a commit that referenced this pull request Jun 2, 2023

contrib: Shift vpp build -> rules_foreign_cc (#27243)

f6e0d02

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: phlax <phlax@users.noreply.github.com>

phlax added a commit that referenced this pull request Jun 2, 2023

contrib: Shift vpp build -> rules_foreign_cc (#27243)

66996e5

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: phlax <phlax@users.noreply.github.com>

phlax added a commit that referenced this pull request Jun 5, 2023

contrib: Shift vpp build -> rules_foreign_cc (#27243)

9c36f79

Signed-off-by: Ryan Northey <ryan@synca.io>

reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023

contrib: Shift vpp build -> rules_foreign_cc (envoyproxy#27243)

fd84b38

Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib: Shift `vpp` build -> `rules_foreign_cc` #27243

contrib: Shift `vpp` build -> `rules_foreign_cc` #27243

phlax commented May 7, 2023

repokitteh-read-only bot commented May 7, 2023

phlax commented May 7, 2023

phlax May 7, 2023

phlax May 7, 2023

phlax May 7, 2023

florincoras May 7, 2023 •

edited

Loading

phlax May 7, 2023 •

edited

Loading

phlax May 7, 2023

keith May 8, 2023

phlax commented May 7, 2023 •

edited

Loading

phlax May 7, 2023

florincoras May 7, 2023 •

edited

Loading

phlax May 7, 2023

phlax May 7, 2023

phlax May 8, 2023

RyanTheOptimist left a comment

phlax commented May 8, 2023

keith May 8, 2023

florincoras left a comment

		**kwargs):
		cache_entries.update({"CMAKE_BUILD_TYPE": "Bazel"})

contrib: Shift vpp build -> rules_foreign_cc #27243

contrib: Shift vpp build -> rules_foreign_cc #27243

Conversation

phlax commented May 7, 2023

repokitteh-read-only bot commented May 7, 2023

phlax commented May 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florincoras May 7, 2023 • edited Loading

Choose a reason for hiding this comment

phlax May 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlax commented May 7, 2023 • edited Loading

Choose a reason for hiding this comment

florincoras May 7, 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

RyanTheOptimist left a comment

Choose a reason for hiding this comment

phlax commented May 8, 2023

Choose a reason for hiding this comment

florincoras left a comment

Choose a reason for hiding this comment

contrib: Shift `vpp` build -> `rules_foreign_cc` #27243

contrib: Shift `vpp` build -> `rules_foreign_cc` #27243

florincoras May 7, 2023 •

edited

Loading

phlax May 7, 2023 •

edited

Loading

phlax commented May 7, 2023 •

edited

Loading

florincoras May 7, 2023 •

edited

Loading