Page MenuHomePhabricator

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

Authored by max on Aug 11 2022, 3:56 AM.
Tags
None
Referenced Files
F3269962: D4805.id15736.diff
Sat, Nov 16, 4:14 AM
F3269893: D4805.id15954.diff
Sat, Nov 16, 4:12 AM
F3269669: D4805.id15545.diff
Sat, Nov 16, 4:08 AM
Unknown Object (File)
Wed, Nov 13, 3:53 AM
Unknown Object (File)
Sat, Nov 9, 10:46 AM
Unknown Object (File)
Sat, Nov 9, 9:00 AM
Unknown Object (File)
Sat, Nov 9, 3:55 AM
Unknown Object (File)
Fri, Nov 8, 4:39 PM

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
max marked 2 inline comments as done.
max edited the test plan for this revision. (Show Details)

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

max added inline comments.
services/lib/cmake-components/corrosion-cxx.cmake
26 ↗(On Diff #15591)

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 ↗(On Diff #15591)

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 ↗(On Diff #15591)

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 ↗(On Diff #15591)

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 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.

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 ↗(On Diff #15591)

oh, it's a stub to make cmake happy

117 ↗(On Diff #15591)

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

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.

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 marked an inline comment as done.
max added inline comments.
services/lib/cmake-components/corrosion-cxx.cmake
88 ↗(On Diff #15591)

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 ↗(On Diff #15591)

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...

jon requested changes to this revision.Aug 18 2022, 7:40 AM
jon added inline comments.
services/lib/cmake-components/corrosion-cxx.cmake
117 ↗(On Diff #15591)

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 marked 2 inline comments as done.
max added inline comments.
services/lib/cmake-components/corrosion-cxx.cmake
117 ↗(On Diff #15591)

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.

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.

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

varun requested changes to this revision.Aug 18 2022, 12:35 PM

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
max marked an inline comment as done.

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

max marked an inline comment as done.
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.

jon requested changes to this revision.Aug 22 2022, 1:16 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 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 ))

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

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

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)?

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.

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.

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