corrosion-rs / corrosion

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

corrosion_set_features did not work #525

Closed flaviojs closed 4 months ago

flaviojs commented 4 months ago

I have a cargo workspace with multiple crates.

Originally I called corrosion_import_crate for the staticlib crate directly, passed features with it, and everything worked.

After adding a bin crate to the workspace I had to change corrosion_import_crate to import the workspace. Since the bin crate does not have the same features, I also had to pass the staticlib features with corrosion_set_features. Unfortunately the staticlib is not getting the features...

jschwe commented 4 months ago

Could you clarify what exactly is happening? Corrosion infernally uses Cargo, and cargo does something called feature unification in workspaces.

flaviojs commented 4 months ago

Illustrative examples:

set ( RUST_FEATURES )
foreach( _feature IN ITEMS ENABLE_X ENABLE_Y ENABLE_Z )
  if( ${_feature} )
    list ( APPEND RUST_FEATURES "${_feature}" )
  endif()
endforeach()

staticlib_crate receives features (OK)

corrosion_import_crate ( MANIFEST_PATH "${CMAKE_SOURCE_DIR}/rust/staticlib_crate/Cargo.toml" FEATURES ${RUST_FEATURES} )

bin_crate complains about features unless i also add then to it's Cargo.toml (BAD)

corrosion_import_crate ( MANIFEST_PATH "${CMAKE_SOURCE_DIR}/rust/Cargo.toml" CRATES staticlib_crate bin_crate FEATURES ${RUST_FEATURES} )

staticlib_crate does not receive features (BAD)

corrosion_import_crate ( MANIFEST_PATH "${CMAKE_SOURCE_DIR}/rust/Cargo.toml" CRATES staticlib_crate bin_crate )
corrosion_set_features ( staticlib_crate FEATURES ${RUST_FEATURES} )

edit: maybe this is related... the two crates are independent. bin_crate does not depend on staticlib_crate

jschwe commented 4 months ago

What cmake version are you using?

flaviojs commented 4 months ago

cmake 3.22.1

jschwe commented 4 months ago

bin_crate complains about features unless i also add then to it's Cargo.toml (BAD)

That is expected. The FEATURES argument only is intended for single crates and less so for workspaces.

For me, your example code fails to update the RUST_FEATURES variable. I don't quite follow what you want to achieve with the if.

set ( RUST_FEATURES )
foreach( _feature IN ITEMS ENABLE_X ENABLE_Y ENABLE_Z )
  if( ${_feature} )
    list ( APPEND RUST_FEATURES "${_feature}" )
  else()
        # fails with a fatal error on my machine.
        message(FATAL_ERROR "if evaluates to false")
  endif()
endforeach()

FYI, here is the test project that Corrosion has for enabling features: https://github.com/corrosion-rs/corrosion/blob/master/test/features/features/CMakeLists.txt There is no bin crate in this constellation, but it shouldn't play a role either.

flaviojs commented 4 months ago

Right, the features are originally cmake options that can be enabled and disabled.

option ( ENABLE_X "describe what X does" ON )
option ( ENABLE_Y "describe what Y does" ON )
option ( ENABLE_Z "describe what Z does" ON )

I was lazy and just named the rust features exactly the same. In corrosion-rs you "enable" features by placing them after FEATURES. The foreach loop tests if each option is enabled, collecting enabled ones to the temporary variable RUST_FEATURES, which is expanded after FEATURES.

I'll see if I can adapt the features test you pointed out to replicate the problem.

flaviojs commented 4 months ago

I adapted the features test and was not able to reproduce.

Just in case I deleted the build dir of my code and reconfigured, now it works. So I guess this is a false positive, and the cause was probably some temporary file from previous attempts. They are deleted so I can't replicate the issue to find out more. Close the issue?

jschwe commented 4 months ago

Okay, great to hear that it works for you now.