search

corrosion-rs / corrosion

Marrying Rust and CMake - Easy Rust and C/C++ Integration!
https://corrosion-rs.github.io/corrosion/
MIT License
1.09k stars 105 forks source link

Support installing libraries #415

Open alex-semov opened 1 year ago

alex-semov commented 1 year ago

Current Behavior

When attempting to use a Rust library my_lib in a C++ application my_app, using Corrosion, the linker reports an error during the linking phase.

Error Output:

[ 59%] Linking CXX executable my_app
ld: cannot find -lmy_lib-static

Relevant issue: https://github.com/corrosion-rs/corrosion/issues/261#issuecomment-1307223603

Expected Behavior

The my_app application should successfully link with the my_lib Rust library without the need for renaming the output static library file.

I was able to temporarily resolve this issue by renaming the output static library as follows:

install(
  FILES
    ${CMAKE_CURRENT_BINARY_DIR}/libmy_lib.a
  DESTINATION
    ${lib_dest}
  RENAME
    libmy_lib-static.a
)

Steps To Reproduce

Here is a snippet of the CMakeLists.txt file which reproduces the issue:

corrosion_import_crate(
  MANIFEST_PATH ${CARGO_MANIFEST_DIR}/Cargo.toml
  CRATES my_lib
)

target_link_libraries(my_app my_lib)

install(
  TARGETS
    my_lib
  EXPORT
    MyTargets
  DESTINATION
    ${lib_dest}/
)

cmake -S. -Bbuild -DRust_CARGO_TARGET=mips-unknown-linux-gnu ...

Environment

- OS: Docker, debian:buster-slim
- CMake: 3.25.0
- CMake Generator: Unix Makefiles
- Cross Compilation Toolchain: mips-unknown-linux-gnu

CMake configure log with Debug log-level

No response

CMake Build step log

No response

jschwe commented 1 year ago

To clarify: the install step is essential to reproduce this issue, correct?

alex-semov commented 1 year ago

Yes, the install step is essential to reproduce this issue. It seems that Corrosion is appending a -static suffix to the target name for static libraries, which is causing a mismatch during the linking phase because the library with the -static suffix is not being installed.

https://github.com/corrosion-rs/corrosion/blob/fc8dd409ba168c6a24572c72cf076154b913ebaa/cmake/Corrosion.cmake#L456C10-L456C10

if(has_staticlib)
    add_library(${target_name}-static STATIC IMPORTED GLOBAL)
    add_dependencies(${target_name}-static cargo-build_${target_name})
...
target_link_libraries(${target_name} INTERFACE ${target_name}-static)
jschwe commented 1 year ago

Using an installed library is curenntly not supported, but it would make sense to add support. I don't have much experience with install, so it would immensely help if you could provide a minimal example of a project, that imports a Rust project, installs and then uses the installed static library to link. This would allow me to test out different solutions on that demo project.

It seems that Corrosion is appending a -static suffix

Corrosion creates an INTERFACE target that links to either the -static or -shared library . Changing this in any way would likely break lots of existing users code.

kkartaltepe commented 8 months ago

INTERFACE libraries basically cannot be installed as far as I can tell, but IMPORTED'ed libraries have some minor support. And in-fact with the existing code you can sort of kinda install to the degree cmake allows installing prebuilt binaries.

get_target_property(myrustlib_import_target myrustlib INTERFACE_LINK_LIBRARIES) 
install(IMPORTED_RUNTIME_ARTIFACTS ${myrustlib_import_target}
LIBRARY DESTINATION ...
)

This is pretty limited and actually doesnt even support headers either, but is documented https://cmake.org/cmake/help/latest/command/install.html#imported-runtime-artifacts

Hilariously INTERFACE libraries supposedly do support installing public headers per https://discourse.cmake.org/t/interface-library-not-getting-installed-and-no-error-reported/2185/22 (See the 3.19 test cases and the newer FILE_SET features)

Hopefully these workarounds help someone else, but it does not look very good for being able to treat corrosion targets like real first-class cmake targets as it stands. Beyond simple installs both INTERFACE and IMPORTED targets have many other limitations you might experience in a large cmake build.

flaviojs commented 5 months ago

Thanks. I was looking for a way to install a bin crate executable (converted from C to rust) and install(IMPORTED_RUNTIME_ARTIFACTS target_name) was enough to replace the original install_executable(target_name).

Mentioning how to do this in the docs would be helpful. :wink:

gtker commented 3 months ago

I created a repo for a library that works both with install and add_subdirectory. It takes an annoying amount of effort, but it shouldn't be impossible to upstream into the library.

Headline is that

if(BUILD_SHARED_LIBS)
    install(FILES $<TARGET_PROPERTY:is_odd-shared,IMPORTED_LOCATION> DESTINATION lib)
else()
    install(FILES $<TARGET_PROPERTY:is_odd-static,IMPORTED_LOCATION> DESTINATION lib)
endif()

will copy the library.

It seems this issue is tightly coupled to https://github.com/corrosion-rs/corrosion/issues/64, https://github.com/corrosion-rs/corrosion/issues/117, and https://github.com/corrosion-rs/corrosion/issues/63.

Ideally I think getting corrosion_install working would be the solution to this issue. I wouldn't mind taking a shot at it, although I don't think I'm familiar enough with the particulars of how the base install works to get it right for all platforms.

SanderVocke commented 3 months ago

@gtker I am unable to reproduce your solution.

With a cdylib crate:

corrosion_import_crate(MANIFEST_PATH Cargo.toml CRATES mycrate)

I am not able to get to the generated .so through any of the properties IMPORTED_LOCATION, LOCATION, INTERFACE_LINK_LIBRARIES, etc. of the mycrate target, and their respective generator expressions in the "install" command.

EDIT: in my case (not sure why), there is an intermediate target generated called mycrate-shared. get_target_property(SHARED_TARGET mycrate INTERFACE_LINK_LIBRARIES) yields mycrate-shared, then the install command can be done with the generator expression FILES $<TARGET_PROPERTY:mycrate-shared,IMPORTED_LOCATION

gtker commented 3 months ago

EDIT: in my case (not sure why), there is an intermediate target generated called mycrate-shared. get_target_property(SHARED_TARGET mycrate INTERFACE_LINK_LIBRARIES) yields mycrate-shared, then the install command can be done with the generator expression FILES $<TARGET_PROPERTY:mycrate-shared,IMPORTED_LOCATION

I believe this is always the case as mentioned in https://github.com/corrosion-rs/corrosion/issues/415#issuecomment-1623087997 although it would probably be considered an implementation detail and would be subject to change at some point, which is why it would be great to have a working corrosion_install that could do all this.

jschwe commented 3 months ago

Pull requests improving corrosion_install are definitely welcome! I haven't really touched that part myself, so I'm also not very familiar with that code.

if(BUILD_SHARED_LIBS)
    install(FILES $<TARGET_PROPERTY:is_odd-shared,IMPORTED_LOCATION> DESTINATION ${shared_lib_dest})
else()
    install(FILES $<TARGET_PROPERTY:is_odd-static,IMPORTED_LOCATION> DESTINATION ${static_lib_dest})
endif()

Some comments about this:

Example (for the first comment about testing if a target exists):

if(TARGET ${target_name}-shared)
    install(FILES $<TARGET_PROPERTY:is_odd-shared,IMPORTED_LOCATION> DESTINATION lib)
endif()
if(TARGET ${target_name}-static)
    install(FILES $<TARGET_PROPERTY:is_odd-static,IMPORTED_LOCATION> DESTINATION lib)
endif()
gtker commented 3 months ago

Pull requests improving corrosion_install are definitely welcome!

Is it OK if I don't create tests for the current corrosion_install and new functionality? I've looked at the test harness and it looks pretty complicated, and as far as I can see there isn't something testing install that I can copy from.

BUILD_SHARED_LIBS only controls which library type is linked to the main INTERFACE target if both the static and shared library are built.

BUILD_SHARED_LIBS is (supposed to be) the standardized Cmake way of building shared libraries, so I just used that. Currently in my Cargo.toml I build both a static and shared lib, what is the intended Corrosion way of selecting static/shared for example in the use case of building a library and installing it? (I'll use your example for the actual implementation since it's way better)

Perhaps something like install(IMPORTED_RUNTIME_ARTIFACTS could be useful when installing shared libraries

On Windows there are also import libraries that you perhaps also need to install along with the dll.

I'll take a look. In the snippet I just used the same format for both shared and static to make it easier to understand what was going on.

Ideally I would want people to be able to add headers to the "top level" corrosion target and have that also be installed correctly to make add_subdirectory/find_package behave the same way.

Would it be OK if I submitted PRs of partial features? Like having it install the library files, then the headers, then configs, and so on.

gtker commented 3 months ago

The way I see it there's a few things that need to be done for this be complete (which would also fix https://github.com/corrosion-rs/corrosion/issues/64, and possibly also https://github.com/corrosion-rs/corrosion/issues/117):

I will try to get through all of these, although I'm not sure when it will be done.