Details

Reviewers

• karol
varun
tomek
• jon

Commits

rCOMMd488b5bc4f82: [services] Add CMake file with functions for Corrosion and CXX integration

Summary

This diff introduces CMake file with functions to integrate Rust library code into CMake project using Corrosion and CXX.
Inspired by the https://github.com/trondhe/rusty_cmake but with the following modifications:

Windows-related code was removed.
Using of the get_filename_component to get stem instead of _get_stem_name_of_path function.
Check for an empty Rust_CARGO_TARGET.
Code fixes according to our code standard.

The purposes of functions:

add_library_rust: Running corrosion and integrating CXX headers and bindings.

Related Linear task: ENG-1438

Test Plan

This diff can be tested in a stack diff D4807.
Successfully built the service, and Rust library, and link it in test in D4807 (end of the stack).

Diff Detail

Repository

rCOMM Comm

Branch

add-cmake-for-cxx-corrosion

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Removing of _get_stem_name_of_path function in a favor of just use of cmake_path.

Harbormaster failed remote builds in B11423: Diff 15715!Aug 17 2022, 7:02 AM

• max marked 2 inline comments as done.Aug 17 2022, 7:04 AM

• max added inline comments.

services/lib/cmake-components/corrosion-cxx.cmake
26	are we able to re-use any of the corrosion variables? https://github.com/corrosion-rs/corrosion#information-provided-by-corrosion Unfortunately, there is no target architecture, only version numbers ((
88	what is this doing? usually `add_custom_command` has a COMMAND argument where some action is performed to create what is specified in OUTPUT That comes from the original rusty_cmake. Looks weird without COMMAND according to the documentation. If we remove it the next `add_library(${_LIB_PATH_STEM}_cxxbridge)` fails with the following error: CMake Error at /Users/max/GitHub/comm/services/lib/cmake-components/corrosion-cxx.cmake:88 (add_library): Cannot find source file: /Users/max/GitHub/comm/services/tunnelbroker/build/cargo/build/aarch64-apple-darwin/cxxbridge/rust/cxx.h Tried extensions .c .C .c++ .cc .cpp .cxx .cu .mpp .m .M .mm .ixx .cppm .h .hh .h++ .hm .hpp .hxx .in .txx .f .F .for .f77 .f90 .f95 .f03 .hip .ispc Call Stack (most recent call first): CMakeLists.txt:76 (add_library_rust) CMake Error at /Users/max/GitHub/comm/services/lib/cmake-components/corrosion-cxx.cmake:88 (add_library): No SOURCES given to target: rust-notifications_cxxbridge Call Stack (most recent call first): CMakeLists.txt:76 (add_library_rust) Relying on this, I think it links headers for the ${_LIB_PATH_STEM} in that weird way.
117	if this is for installation purpose, we should be doing `install(TARGET` on the library we made I'm not sure what you exactly mean here, can you please add some info?
120	Feel like this could be: get_filename_component(_path_dir ${_path} DIRECTORY) get_filename_component(_path_dir ${_path_dir} NAME) example: set(_path "/home/jon/projects/comm/comm/example/directory/lib.rs") get_filename_component(_path_dir ${_path} DIRECTORY) get_filename_component(_path_dir ${_path_dir} NAME) message(STATUS "parent directory: ${_path_dir}") $ cmake .. -- parent directory: directory Good catch, thanks @jon ! It could be even easier using of the cmake_path function. We can remove this function completely in a favor of using just one line command. I've removed the function and use `cmake_path` STEM instead.

• max added 2 blocking reviewer(s): • jon, varun.Aug 17 2022, 7:05 AM

• max edited the summary of this revision. (Show Details)

Adding @jon and @varun as blocking reviewers, because of this cmake file can be used in all services.

• max edited the summary of this revision. (Show Details)Aug 17 2022, 7:10 AM

Variable names should be fixed according to our new CMake Lint to address errors but seems this doesn't block the review.

It could be even easier using of the cmake_path function. We can remove this function completely in a favor of using just one line command.

This might be hard for the docker environment as cmake_path is a very recent addition.

services/lib/cmake-components/corrosion-cxx.cmake
88	oh, it's a stub to make cmake happy
117	I'm not sure what you exactly mean here, can you please add some info? You can export a library under a namespace using `install` https://github.com/CommE2E/comm/blob/42e38d9502a47c8de62fa285148bfea2fe6b984d/native/cpp/CommonCpp/Tools/CMakeLists.txt#L51-L55

• max planned changes to this revision.Aug 17 2022, 9:18 AM

In D4805#140773, @jon wrote:

It could be even easier using of the cmake_path function. We can remove this function completely in a favor of using just one line command.

This might be hard for the docker environment as cmake_path is a very recent addition.

Oh yes, thanks. We have only 3.16 in Ubuntu... I should revert this.

Changes to use get_filename_component instead of cmake_path to support current CMake version.
Fixing cmake-lint errors.
Docstring were added to the function declarations.

• max planned changes to this revision.Aug 18 2022, 7:23 AM

This might be hard for the docker environment as cmake_path is a very recent addition.

I've reverted to using get_filename_component but there is no need to create an additional function anyway. Because we need to get stem from the directory path and the single line get_filename_component work for us:

set(_path "../jon/max")
  get_filename_component(_path_dir ${_path} NAME)
  message(STATUS "last directory: ${_path_dir}")
set(_path "../jon")
  get_filename_component(_path_dir ${_path} NAME)
  message(STATUS "last directory: ${_path_dir}")
set(_path "./jon")
  get_filename_component(_path_dir ${_path} NAME)
  message(STATUS "last directory: ${_path_dir}")
set(_path "jon/max")
  get_filename_component(_path_dir ${_path} NAME)
  message(STATUS "last directory: ${_path_dir}")
set(_path "max")
  get_filename_component(_path_dir ${_path} NAME)
  message(STATUS "last directory: ${_path_dir}")

-- last directory: max
-- last directory: jon
-- last directory: jon
-- last directory: max
-- last directory: max

That's why I just use the get_filename_component(_LIB_PATH_STEM ${lib_path} NAME) in line 75 and removed the stem function at all.

• max requested review of this revision.Aug 18 2022, 7:32 AM

• max marked an inline comment as done.

• max added inline comments.

services/lib/cmake-components/corrosion-cxx.cmake
88	oh, it's a stub to make cmake happy I've added a comment there and an empty `COMMAND` to fix the lint error at line 99.
117	I'm not sure what you exactly mean here, can you please add some info? You can export a library under a namespace using `install` https://github.com/CommE2E/comm/blob/42e38d9502a47c8de62fa285148bfea2fe6b984d/native/cpp/CommonCpp/Tools/CMakeLists.txt#L51-L55 I've checked the `install` and seems that we should `add_library` before it anyway. Sorry, but I'm not getting the sense to add `install` here. After `add_library` the library links to the app and can be used inside it. What is the purpose of adding `install` here? Maybe I've missed something...

Harbormaster completed remote builds in B11441: Diff 15736.Aug 18 2022, 7:33 AM

• jon requested changes to this revision.Aug 18 2022, 7:40 AM

• jon added inline comments.

services/lib/cmake-components/corrosion-cxx.cmake
117	oh, this was in regards to the `add_library(... ALIAS)` we have. Which is doing largely what `install(TARGETS` would do, which is export a library under a given namespace. Phabricator isn't updating to the new position between diffs.

This revision now requires changes to proceed.Aug 18 2022, 7:40 AM

• max requested review of this revision.Aug 18 2022, 8:23 AM

• max marked 2 inline comments as done.

• max added inline comments.

services/lib/cmake-components/corrosion-cxx.cmake
117	oh, this was in regards to the `add_library(... ALIAS)` we have. Which is doing largely what `install(TARGETS` would do, which is export a library under a given namespace. Seems that install doesn't support to mix TARGETS and NAMESPACE: CMake Error at /Users/max/GitHub/comm/services/lib/cmake-components/corrosion-cxx.cmake:125 (install): install TARGETS given unknown argument "NAMESPACE". Phabricator isn't updating to the new position between diffs. I've updated D4807 on recent changes and it can be tested from there.

sorry, I meant install(EXPORT https://cmake.org/cmake/help/latest/command/install.html#installing-exports

which is what https://github.com/CommE2E/comm/blob/42e38d9502a47c8de62fa285148bfea2fe6b984d/native/cpp/CommonCpp/Tools/CMakeLists.txt#L51-L55 is doing

that being said, it's hard to use install(EXPORT when using add_subdirectory. Essentially someone would need to do cmake .. && make install. And then they would be able to use the install(EXPORT contents.

I'm okay with doing add_library(${namespace}::${_LIB_PATH_STEM} ALIAS ${_LIB_PATH_STEM}-total), I just don't think it's canonical. I believe https://cmake.org/cmake/help/latest/command/export.html#targets is the correct thing to use. I just have no idea how to actually use the exported file, as it's not generated until after cmake configure.

ashoat added inline comments.Aug 18 2022, 10:27 AM

services/lib/cmake-components/corrosion-cxx.cmake
79 ↗	(On Diff #15736)	Talked about this offline with @max and @varun, but I'm confused why we need to create a directory specifically for each architecture. Would be great to get an answer here... might be that the author of `rusty_cmake` was trying to support builds with multiple architectures, which we might not need to support

I think max is planning some changes to this diff so i'm pushing it back to his queue

This revision now requires changes to proceed.Aug 18 2022, 12:35 PM

Removing of detect_cargo_target_architecture.
Check for an empty Rust_CARGO_TARGET was added.

• max planned changes to this revision.Aug 19 2022, 3:19 AM

• max edited the summary of this revision. (Show Details)Aug 19 2022, 3:21 AM

Harbormaster completed remote builds in B11476: Diff 15787.Aug 19 2022, 3:29 AM

In D4805#141119, @jon wrote:

that being said, it's hard to use install(EXPORT when using add_subdirectory. Essentially someone would need to do cmake .. && make install. And then they would be able to use the install(EXPORT contents.

I'm okay with doing add_library(${namespace}::${_LIB_PATH_STEM} ALIAS ${_LIB_PATH_STEM}-total), I just don't think it's canonical. I believe https://cmake.org/cmake/help/latest/command/export.html#targets is the correct thing to use.
I just have no idea how to actually use the exported file, as it's not generated until after cmake configure.

It's successfully used without install, just add_library in a sandbox playground repo. I've updated it with the current cmake changes so you can test it easily.
The rust lib functions are successfully called from the C++ in that example and built in our D4807 (I've updated it also). Seems it does not make sense to install here.

services/lib/cmake-components/corrosion-cxx.cmake
79 ↗	(On Diff #15736)	Talked about this offline with @max and @varun, but I'm confused why we need to create a directory specifically for each architecture. Would be great to get an answer here... might be that the author of `rusty_cmake` was trying to support builds with multiple architectures, which we might not need to support Thanks @ashoat, for a good question! I've created a separate task ENG-1672 to track discussion for this integration. After figuring it out with @varun we have found that `Rust_CARGO_TARGET` is used for cross-compilation and we can omit to provide it by default. The `detect_cargo_target_architecture` function should be removed. It was removed from the current diff. For a cross-compilation (if will need it), we can use something like this but updated with the darwin arm architecture for the apple arm. Makes sense to do this as a follow-up update if would need it.

In D4805#141290, @varun wrote:

I think max is planning some changes to this diff so i'm pushing it back to his queue

Yeah, I've updated this diff with the changes after our sync and ENG-1672 as well. Thanks, @varun !

Going to resign for now because I'm not a good person to review this diff. Feel free to add me again when this diff is accepted by other reviewers to give a final review.

Is it possible to move this to the project root directory so I can use it in native as well?

In D4805#141985, @varun wrote:

Is it possible to move this to the project root directory so I can use it in native as well?

I think we can do this, but we don't have a folder for such things yet. We are sharing the protobuf files from native/cpp/CommonCpp.
We can use something like native/cpp/CommonCpp/CMake/corrosion-cxx.cmake. What do you think @varun? Also cc to @jon, @karol, @tomek.

In D4805#141989, @max wrote:

In D4805#141985, @varun wrote:

Is it possible to move this to the project root directory so I can use it in native as well?

I think we can do this, but we don't have a folder for such things yet. We are sharing the protobuf files from native/cpp/CommonCpp.
We can use something like native/cpp/CommonCpp/CMake/corrosion-cxx.cmake. What do you think @varun? Also cc to @jon, @karol, @tomek.

Sounds good to me

Moving cmake file to the native/cpp/CMake folder.

In D4805#142010, @jon wrote:

In D4805#141989, @max wrote:

In D4805#141985, @varun wrote:

Is it possible to move this to the project root directory so I can use it in native as well?

I think we can do this, but we don't have a folder for such things yet. We are sharing the protobuf files from native/cpp/CommonCpp.
We can use something like native/cpp/CommonCpp/CMake/corrosion-cxx.cmake. What do you think @varun? Also cc to @jon, @karol, @tomek.

Sounds good to me

@varun Is it possible to move this to the project root directory so I can use it in native as well?

I just realized that we have only c++ sources/headers in native/cpp/CommonCpp.
That's why the better place would be native/cpp/CMake.
I've moved this file to this folder.

Harbormaster completed remote builds in B11512: Diff 15833.Aug 22 2022, 12:04 PM

Otherwise LGTM.

Might do some upstream work with corrosion-rs to make this less awkward.

native/cpp/CMake/corrosion-cxx.cmake
79–80 ↗	(On Diff #15833)	is there a reason to have a separate `target_sources`? IIRC, this shouldn't be necessary for normal libraries

This revision now requires changes to proceed.Aug 22 2022, 1:16 PM

Removing target_sources in favor of add_library only.

• max marked an inline comment as done.Aug 22 2022, 5:00 PM

• max added inline comments.

native/cpp/CMake/corrosion-cxx.cmake
79–80 ↗	(On Diff #15833)	is there a reason to have a separate `target_sources`? IIRC, this shouldn't be necessary for normal libraries We can remove it. I've tested it in a playground and can confirm that we can use only `add_library` here. I've removed `target_sources` in a favor of `add_library` only. Thanks, @jon! It looks beautiful now ))

Harbormaster completed remote builds in B11520: Diff 15842.Aug 22 2022, 5:09 PM

I think we should have a separate top-level directory that contains Rust/C++/protos shared between native, services, and web

In D4805#142222, @ashoat wrote:

I think we should have a separate top-level directory that contains Rust/C++/protos shared between native, services, and web

Yeah this seems to be the pattern I see in other repos

In D4805#142222, @ashoat wrote:

I think we should have a separate top-level directory that contains Rust/C++/protos shared between native, services, and web

What about cpp/CMake for this file and the same CMake stuff and moving all from native/cpp to the root cpp directory (this will take a while to do, to fix all the includes everywhere, that's why it can be as a follow-up task)?

• jon accepted this revision.Aug 23 2022, 8:46 AM

This revision is now accepted and ready to land.Aug 23 2022, 8:46 AM

Deferring this diff until consensus in a dev team chat thread for the new shared items directory structure and then move this file though.

Moving cmake file to the /shared/cmake folder after we have a consensus in a chat thread about the shared directory structure.

Harbormaster completed remote builds in B11586: Diff 15928.Aug 24 2022, 7:30 AM

• max added 1 blocking reviewer(s): tomek.Aug 24 2022, 8:45 AM

This revision now requires review to proceed.Aug 24 2022, 8:45 AM

In D4805#141578, @tomek wrote:

Going to resign for now because I'm not a good person to review this diff. Feel free to add me again when this diff is accepted by other reviewers to give a final review.

Adding @tomek for a final review before landing it.

• max mentioned this in D4949: [services] Tunnelbroker - Add copying of the shared directory to the Dockerfile.Aug 24 2022, 5:01 PM

• max added a child revision: D4949: [services] Tunnelbroker - Add copying of the shared directory to the Dockerfile.Aug 24 2022, 5:11 PM

tomek accepted this revision.Aug 25 2022, 1:10 AM

This revision is now accepted and ready to land.Aug 25 2022, 1:10 AM

Closed by commit rCOMMd488b5bc4f82: [services] Add CMake file with functions for Corrosion and CXX integration (authored by • max). · Explain WhyAug 25 2022, 1:33 AM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMMd488b5bc4f82: [services] Add CMake file with functions for Corrosion and CXX integration.

• max removed a child revision: D4807: [services] Tunnelbroker - Update CMakeLists and Cargo build with the CXX integration updates.Aug 29 2022, 5:48 AM

• max mentioned this in rCOMM13f2e1f40883: [services] Tunnelbroker - Update CMakeLists and Cargo build with the CXX….Sep 6 2022, 5:21 AM