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

Add LINKER_LANGUAGE option to corrosion_import_crate #188

Closed
wants to merge 2 commits into from

Conversation

amunra
Copy link

@amunra amunra commented Jun 13, 2022

Background and Linker Problem

I am developing a library that is intended to be consumed by both C and C++.
The library's core logic is written in Rust, exporting a .h C header and an optional wrapper .hpp C++ header, with tests in C++.

I've started using corrosion to integrate calling cargo with CMake, but noticed (investigating with ldd on Linux and otool -L on MacOS) that the corrosion-cargo-built binary depended on the C++ standard library.

The C++ dependency is absent when invoking cargo on the command line directly and is introduced by corrosion when it sets the CORROSION_LINKER_PREFERENCE variable cmake/Corrosion.cmake which is eventually passed to cargo as CARGO_TARGET_...._LINKER=path/to/cpp_compiler. Reading the code, this logic is skipped when building on Windows with MSVC.

It is a hard requirement for my project that the rust-built library does not depend on C++, as such I've started adding a feature to corrosion to specify which language's linker to pass into the cargo invocation.

Approach

The approach I'm taking is to add an optional LINKER_LANGUAGE parameter to corrosion_import_crate. For my needs, this ends up looking like so:

corrosion_import_crate(
    MANIFEST_PATH Cargo.toml
    FEATURES ffi
    LINKER_LANGUAGE C)

Does this make sense?

P.S.: I've also noticed there's a corrosion_set_linker_language CMake function, but it didn't seem to do anything relevant to my problem.

State of this pull request

I'm raising a draft pull request because, frankly, I'm still learning CMake and I expect more changes are needed before this can be merged in.

Testing

So far I've re-run a subset of the existing tests locally on Linux as so:

# Prepare for testing
rm -fR build
cmake \
  -S . \
  -B build \
  -DCORROSION_VERBOSE_OUTPUT=ON \
  -DCMAKE_C_COMPILER=gcc \
  -DCMAKE_CXX_COMPILER=g++ \
  -GNinja

# Run Config Tests (x86_64 Only)
(cd build &&
cmake \
  --build . --verbose \
  --target test-install
ctest --verbose --build-config Debug)

# Build C++ program linking Rust
(cmake \
  --build build --verbose \
  --target cpp-exe)

# Build Rust program linking C++
(cmake \
  --build build --verbose \
  --target cargo-build_rust-exe)

# Build Rust cdylib and link against C++
(cmake \
  --build build --verbose \
  --target my_program)

This is not exhausitve, but I think it's unlikely I've broken anything.

I certainly haven't tested the new feature though, aside from the low bar of "it works for me".

I would appreciate some guidance on how to progress, especially on the testing front:

  • Would this be a new directory under test/?
  • Should I create different tests to with and without the native tools or is this somehow handled by the github actions workflow matrix?
  • Any idea on how to actually assert if a cargo built library doesn't have a C++ dependency from CMake -- pretty clueless here.

Docs

There's also a matter of documentation: The "LINKER_LANGUAGE" feature would be de facto Mac/Linux only.
Is it OK to just document (and maybe log) that the named argument is ignored on Windows/MSVC?

Naming

Should the parameter name here be changed? Would LINK_AS be a better name to avoid creating confusion with the already existing corrosion_set_linker_language cmake function (that I don't really understand the intended purpose of)?

Thanks!

…s to allow a crate to not have dependencies on the C++ standard lib when compiled from corrosion.
@jschwe
Copy link
Collaborator

jschwe commented Jun 14, 2022

The C++ dependency is absent when invoking cargo on the command line directly and is introduced by corrosion when it sets the CORROSION_LINKER_PREFERENCE variable cmake/Corrosion.cmake which is eventually passed to cargo as CARGO_TARGET_...._LINKER=path/to/cpp_compiler.

Corrosion currently will select the linker with the highest priority (as determined by CMake). So if you have C++ enabled as a language in your project, The C++ compiler will be selected. (I don't know why it's different for MSVC).

The approach I'm taking is to add an optional LINKER_LANGUAGE parameter to corrosion_import_crate

This should not be a parameter for corrosion_import_crate but rather a target based property.

P.S.: I've also noticed there's a corrosion_set_linker_language CMake function, but it didn't seem to do anything relevant to my problem.

Yes, this function does not work as intended. I wrote up how I view the problem in issue #133. But I got busy with other stuff, so I never ended up fixing it.

As described in the issue I would prefer if we had a new corrosion_set_linker function, which sets a target property (the name should not be LINKER_LANGUAGE, since that is an existing CMake target property). Then when setting CARGO_TARGET_...._LINKER we should set it to the value of the new target property if set, and otherwise set it to the same value as before (i.e. the linker with the highest priority).

I think this approach would also fix your problem. Would you be interested in doing this?

@jschwe
Copy link
Collaborator

jschwe commented Jun 14, 2022

As for testing:

Would this be a new directory under test/?

Yes

Should I create different tests to with and without the native tools or is this somehow handled by the github actions workflow matrix?

This is handled by our github actions matrix.

Any idea on how to actually assert if a cargo built library doesn't have a C++ dependency from CMake -- pretty clueless here

You could try to call a C++ standard library function. This should cause a failure at the linking step if the C++ stdlib is not linked. Then you just have to tell ctest that you expected a build-failure for this test - I think this stack overflow answer might point you in the right direction: https://stackoverflow.com/a/30191576

@amunra
Copy link
Author

amunra commented Jun 14, 2022

The C++ dependency is absent when invoking cargo on the command line directly and is introduced by corrosion when it sets the CORROSION_LINKER_PREFERENCE variable cmake/Corrosion.cmake which is eventually passed to cargo as CARGO_TARGET_...._LINKER=path/to/cpp_compiler.

Corrosion currently will select the linker with the highest priority (as determined by CMake). So if you have C++ enabled as a language in your project, The C++ compiler will be selected. (I don't know why it's different for MSVC).

Yes, that's what I noticed. I'm assuming there's no way in CMake to disable C++ for just part of the project. It's also quite surprising that a corrosion uses a different linker than the default one. I'm not sure why it goes through the effort rather than just letting cargo select it: I'm sure there's a good reason for it though :-).

The approach I'm taking is to add an optional LINKER_LANGUAGE parameter to corrosion_import_crate

This should not be a parameter for corrosion_import_crate but rather a target based property.

P.S.: I've also noticed there's a corrosion_set_linker_language CMake function, but it didn't seem to do anything relevant to my problem.

Yes, this function does not work as intended. I wrote up how I view the problem in issue #133. But I got busy with other stuff, so I never ended up fixing it.

As described in the issue I would prefer if we had a new corrosion_set_linker function, which sets a target property (the name should not be LINKER_LANGUAGE, since that is an existing CMake target property). Then when setting CARGO_TARGET_...._LINKER we should set it to the value of the new target property if set, and otherwise set it to the same value as before (i.e. the linker with the highest priority).

I think this approach would also fix your problem. Would you be interested in doing this?

I'd be happy to give it a try, but as I mentioned this is my first time using CMake: I would need some pointers and assistance. In the meantime I need the feature urgently and I'll be using my forked repo.

I'm also unclear on what the benefit is of having this be a per-target function: My understanding is that a rust crate may have exactly 0 or 1 libraries defined. It can however have multiple binaries, though these would all be built as part of the same invocation of cargo with the same linker. I'm unclear how a per-target approach would work in practice.

@jschwe
Copy link
Collaborator

jschwe commented Sep 1, 2022

#208 added an option to explicitly set the linker for a target, which should also fit your usecase.

@jschwe jschwe closed this Sep 1, 2022
@amunra
Copy link
Author

amunra commented Sep 18, 2023

Hi @jschwe,

I know it's an old PR, but I just now finally have the cycles to move off my fork and onto your most recent changes.

I've tried using the new feature as suggested, but I couldn't quite figure out how corrosion_set_linker is supposed to work.

This is what I tried:

# Imports `questdb_client` target.
add_subdirectory(corrosion)
corrosion_import_crate(
    MANIFEST_PATH questdb-rs-ffi/Cargo.toml)
corrosion_set_linker(questdb_client C)

The error I get is:

error: linker `C` not found
  |
  = note: No such file or directory (os error 2)

If I compile with cmake --build build -v it looks like it invokes cargo as so:

/usr/bin/cmake -E env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER=C [...] -- -Cdefault-linker-libraries=yes

I guess what I really need to pass is the path to the actual linker to use.
Is there a separate function for this?

corrosion_set_linker(questdb_client is_there_a_get_linker_for_language_fn_somewhere(C))

More broadly, why is this even a thing? Why even pass a linker in the first place and not just let cargo use its default linker, which will be the C linker by default? Should I expect something to break?

@jschwe
Copy link
Collaborator

jschwe commented Sep 19, 2023

I guess what I really need to pass is the path to the actual linker to use.

Correct, you should pass a linker or linker-driver (e.g. clang / gcc or ld.lld ...).
By default Corrosion sets a suitable linker for you (except for windows-msvc, there is only one option there).

More broadly, why is this even a thing? Why even pass a linker in the first place and not just let cargo use its default linker, which will be the C linker by default? Should I expect something to break?

  • If you link in C++ code, the C++ standard library must be linked into the final executable. Rust doesn't know that the code we link in is C++. The C++ standard library name is different for clang and gcc. We could of course determine the correct name, but all that is unnecessary, if we just use the CMAKE_CXX_COMPILER as the linker-driver, since it will automatically add the correct c++ std library to the linker line. Corrosion does this by default if at least 1 C++ library is linked into a Cargo executable.
  • If you are cross-compiling, Rust will select cc as the default linker for many targets. That is just broken because cc will attempt to link for the host target instead of the cross-compile target, and linking will fail. By setting the C (cross-)compiler as the linker-driver Corrosion fixes this issue by default.
  • The former issue does not affect all targets though, and some users might prefer for Rust to directly call ld.lld or some other linker. hence users can use corrosion_set_linker if they are not happy with the default linker selected by corrosion.

@amunra
Copy link
Author

amunra commented Sep 19, 2023

Sorry. I'm still a little confused.

I have Rust code that I want to build both static and dynamic libs from. The Rust code itself does not link C++ code. The Rust code exposes a C API and needs to be callable from both C and C++. The C API does not take callbacks or anything fancy (so no worry about trapped exceptions).

If I call cargo build --release I get the right output by default. If I try and build through corrosion, I get an extra C++ dependency from the produced .so or .dylib that I'm trying to remove.

I'm struggling to determine the right incantation of CMake functions I need to call for this.

For example, this:

corrosion_set_linker(questdb_client ${CMAKE_C_LINK_EXECUTABLE})

also fails with:

error: linker `<CMAKE_C_COMPILER> <FLAGS> <CMAKE_C_LINK_FLAGS> <LINK_FLAGS> <OBJECTS> -o <TARGET> <LINK_LIBRARIES>` not found
  |
  = note: No such file or directory (os error 2)

The library I'm shipping can be built by multiple compilers on multiple OSs, so I don't know how to determine the right linker.

Would you be able to help?

@jschwe
Copy link
Collaborator

jschwe commented Sep 19, 2023

CMAKE_C_LINK_EXECUTABLE is not a linker but a CMake internal. If you want to use the C compiler just use corrosion_set_linker(questdb_client ${CMAKE_C_COMPILER}). But this should be the default if you don't link C++ into your Rust code.

If I try and build through corrosion, I get an extra C++ dependency from the produced .so or .dylib that I'm trying to remove.

The Rust code itself does not link C++ code

Are you sure you don't call corrosion_link_libraries() (which links C/C++ libraries into Rust) ?

@amunra
Copy link
Author

amunra commented Sep 19, 2023

Nice one! You guys fixed it!

It seems that the default behaviour I was worried about which I noticed when I initally forked corrosion-rs (always linking the C++ lib) was removed and it now behaves fine without me doing anything: No need to specify corrosion_set_linker even.

I don't call corrosion_link_libraries() and indeed the newer version of corrosion now works out of the box without further nudging.

Sorry for not re-verifying with the default options 😅!

Made my day! Thanks!

@jschwe
Copy link
Collaborator

jschwe commented Sep 20, 2023

It seems that the default behaviour I was worried about which I noticed when I initally forked corrosion-rs

Yeah, previously simply having C++ enabled as a language would cause corrosion to use the C++ compiler as the linker (which caused the behaviour you saw).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants