Skip to content
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

Support installing libraries #415

Closed
alex-semov opened this issue Jul 4, 2023 · 12 comments
Closed

Support installing libraries #415

alex-semov opened this issue Jul 4, 2023 · 12 comments
Labels
enhancement New feature or request

Comments

@alex-semov
Copy link

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: #261 (comment)

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

@alex-semov alex-semov added the bug Something isn't working label Jul 4, 2023
@jschwe
Copy link
Collaborator

jschwe commented Jul 5, 2023

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

@alex-semov
Copy link
Author

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 jschwe added enhancement New feature or request and removed bug Something isn't working labels Jul 15, 2023
@jschwe jschwe changed the title [Bug]: Linking Error: ld: cannot find -lmy_lib-static. Toolchain: mips-unknown-linux-gnu Support installing libraries Jul 15, 2023
@jschwe
Copy link
Collaborator

jschwe commented Jul 15, 2023

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
Copy link

kkartaltepe commented Feb 17, 2024

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.

@jschwe jschwe removed their assignment Mar 30, 2024
@flaviojs
Copy link

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

@gtker
Copy link
Contributor

gtker commented Jul 28, 2024

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 #64, #117, and #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
Copy link

SanderVocke commented Aug 2, 2024

@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
Copy link
Contributor

gtker commented Aug 2, 2024

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 #415 (comment) 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
Copy link
Collaborator

jschwe commented Aug 2, 2024

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:

  • BUILD_SHARED_LIBS only controls which library type is linked to the main INTERFACE target if both the static and shared library are built. Instead of testing this variable, you probably should instead test if the target exists. Example below.
  • the -shared and -static suffixes for the real IMPORTED libraries are considered an implementation detail. However, I do know that people rely on this implementation detail, so it's very unlikely to change. And if it does change, I would definitely document it in the changelog and tie it to a major version increase.
  • I would also recommend taking a look at recent additions to the install() command. 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.

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
Copy link
Contributor

gtker commented Aug 2, 2024

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 pushed a commit to gtker/corrosion that referenced this issue Aug 2, 2024
Works on corrosion-rs#415.

This only installs the actual library file(s) and not extras such as
headers.

Not tested on Windows for compatibility with import library files for
shared libraries.
@gtker
Copy link
Contributor

gtker commented Aug 3, 2024

The way I see it there's a few things that need to be done for this be complete (which would also fix #64, and possibly also #117):

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

jschwe pushed a commit that referenced this issue Aug 3, 2024
* Make corrosion_install work for library files

Works on #415.

This only installs the actual library file(s) and not extras such as
headers.

Not tested on Windows for compatibility with import library files for
shared libraries.

* Add `corrosion_install` to public docs

Works on #63.

The function definition is currently prescriptive, since it describes
features that don't currently work.

EXPORT causes a FATAL_ERROR, and PRIVATE_HEADER/PUBLIC_HEADER do
nothing.

* Change corrosion_install according to PR feedback

Changes requested in #543.

* Fix typo in corrosion_install docs

Co-authored-by: Tobias Fella <[email protected]>

* Update corrosion_install docstring to remove TODO

---------

Co-authored-by: Gtker <[email protected]>
Co-authored-by: Tobias Fella <[email protected]>
@gtker
Copy link
Contributor

gtker commented Nov 15, 2024

soname documentation was added in this commit so this issue can be closed now.

@jschwe jschwe closed this as completed Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants