-
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
Conversation
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
vpp
build -> rules_foreign_cc
vpp
build -> rules_foreign_cc
cc @florincoras i think this builds a lot unnecessarily but does kinda work (at least testing locally) i tried setting the target to 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 |
contrib/vcl/source/BUILD
Outdated
targets = ["", "install"], | ||
build_data = [requirement("ply")], | ||
env = { | ||
"PLYPATHS": "$(locations %s)" % requirement("ply"), |
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.
there is probably a more elegant way to do this - but i was struggling to think of one
############################################################################## | ||
# vat2 | ||
############################################################################## | ||
-add_vpp_executable(vat2 ENABLE_EXPORTS |
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 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
+ 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 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
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.
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?
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 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
**kwargs): | ||
cache_entries.update({"CMAKE_BUILD_TYPE": "Bazel"}) |
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
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 |
contrib/vcl/source/BUILD
Outdated
&& find . -name "*.a" | xargs -I{} cp -a {} $INSTALLDIR/lib/ \ | ||
&& find . -name "*.h" | xargs -I{} cp -a {} $INSTALLDIR/include | ||
""", | ||
tags = ["skip_on_windows", "cpu:16"], |
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 if this cpu setting does anything
Signed-off-by: Ryan Northey <ryan@synca.io>
vpp
build -> rules_foreign_cc
vpp
build -> rules_foreign_cc
working_directory = "src", | ||
) | ||
|
||
genrule( |
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.
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 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)
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.
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.
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
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 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!
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 running without despite various attempts we didnt manage to get the patch to produce the header as part of a default (build?) run |
**kwargs): | ||
cache_entries.update({"CMAKE_BUILD_TYPE": "Bazel"}) |
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
Signed-off-by: Ryan Northey <ryan@synca.io>
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.
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: phlax <phlax@users.noreply.github.com>
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: phlax <phlax@users.noreply.github.com>
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: 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>
Fix #27236
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:]