-
Notifications
You must be signed in to change notification settings - Fork 17
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
Rust integration (experimental) #99
Conversation
d0734cf
to
6bc4623
Compare
aa7b9df
to
143cd9a
Compare
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
f5d4e32
to
9a81615
Compare
All comments addressed. The cpp linter is wrong btw, |
OUTPUT ${output_dir}lib.rs.h ${output_dir}lib.rs.cc | ||
COMMAND ${CXXBRIDGE} ${CMAKE_CURRENT_SOURCE_DIR}/${module_name}/src/lib.rs --header -o ${output_dir}lib.rs.h | ||
COMMAND ${CXXBRIDGE} ${CMAKE_CURRENT_SOURCE_DIR}/${module_name}/src/lib.rs -o ${output_dir}lib.rs.cc | ||
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/${module_name}/src/lib.rs | ||
COMMENT "Generating cxx for ${module_name}" | ||
VERBATIM |
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.
- rather than calling the cxxbridge util here directly, I would call
cxx_build::bridge
from within thebuild.rs
- I think, it should be the concern of this
cxxrs.cmake
script to figure out the path ofCXXBRIDGE
and not if its including script
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.
- The first one is a chicken and egg though: You would need to run cargo build to get the cpp code, which you need to run cmake to build the static C library, which cargo needs to link against. I.e. you would need to run cargo for only the build.rs file (which I do not think is possible), then run cmake, then cargo again. I think doing it here is the only possibility.
- I do not understand this comment. Can you expand?
everest::log | ||
) | ||
|
||
install(TARGETS everestrs_sys LIBRARY) |
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 wouldn't binary ship this glue code library, as it depends on auto-generated stuff (`rust/cxx.h'), which will only be generated, when its using crate is build.
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 do understand that you do not like the installing piece, however the comment is not actionable to me, I do not know what to do instead: To build any crate using everestrs
, this library must be findable. The only way I know to make it findable is by installing it, that seems the EVerest way to go. If you have a better suggestion, I would be happy to try it out, but likely in a followup MR.
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
7dfb007
to
b6da315
Compare
Thanks for the approval. I have no rights to merge this though, who can help me with this? |
I can merge this, one minor thing would be to run clang-format over the c++ code, we do have a dockerized version of the exact version we require here, but you can see the output as well in the Lint step of the github action. One example command line would be: (maybe adding --user to provider your correct userid/groupid) docker run --volume "$(pwd):/source" [ghcr.io/everest/everest-clang-format:latest](http://ghcr.io/everest/everest-clang-format:latest) -r /source --extensions "hpp,cpp" -e "/source/cache" -e "/source/build" -e "/source/build-cross" -e "/source/build-clang" -i |
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass Thanks for doing the formatting for me. I tried myself and the container does not run for me (MacOS Arm64):
It seems the linking inside the container is not done correctly, though I am confused by the |
Ah that actually makes sense, since our container was initially just meant for the github action it just uses a prebuilt clang-format binary for x86-64. That then obviously doesn't work on arm64 |
This is using cxx.rs to wrap the framework library and calling it from Rust. This code is currently only supporting providing an interface to be implemented, i.e. no variables publish or receiving and no calling of other interfaces. Those features are straightforward, quick and easy to implement, but for now this is probably enough to iron out the integration questions.
See the everestrs/README.md for more details on the implementation.
Co-authored-by: Dima Dorezyuk ddo@qwello.eu