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

Generated headers and CMake install #261

Open jschwe opened 1 year ago

jschwe commented 1 year ago
    @trondhe @jschwe 

I have a question about the usage of this feature. Suppose I have a C++ library my_lib that depends on the cxx_bridge_target, which uses this feature to generate. Currently, the generated headers are placed under ${CMAKE_CURRENT_BINARY_DIR}/corrosion_generated directory, and with some additional sub directory path like cxxbridge/${cxx_target}, which I have to read the corrosion code to figure out.

If my C++ library my_lib needs to be exported and installed for external consumption, I wonder how corrosion users are expected to setup the project for my_lib so that the generated cxx bridge related headers can be installed and consumed by my_lib users. Do you have any guidance on this? Thanks.

Originally posted by @niyue in https://github.com/corrosion-rs/corrosion/issues/244#issuecomment-1306504123

jschwe commented 1 year ago

It looks like this is what corrosion_install was meant for, however it currently only seems to support installing executable files and the rest is not implemented.

As for how you can do it in the meantime:

I would have a look here: https://cmake.org/cmake/help/latest/command/install.html#installing-files and probably also file-sets if you use at least CMake 3.23: https://cmake.org/cmake/help/latest/command/target_sources.html#file-sets

jschwe commented 1 year ago

On the corrosion side I think it would make sense to define a header fileset on CMake >= 3.23 for the generated headers.

msvetkin commented 1 year ago

As far as a user of the corrision_add_cxxbridge have access to ${cxx_target} then it can add all necessary install rules.

The reason why I think we should not add any install rules into corrision_add_cxxbridge is that we don't not if user want to export this headers. The headers could be internal for the project.

@jschwe I think what we should do is set a property on ${cxx_target} which tells where it is generated headers are placed. It will allow users to decide what to do and how to do.

set_target_properties(${cxx_target}
   PUBLIC
        CORROSION_HEADER_DIR ${generated_dir}/include
)
niyue commented 1 year ago

@jschwe @msvetkin Thanks so much for the prompt reply.

It looks like this is what corrosion_install was meant for

This is what I was looking for initially, and as you said, the implementation is incomplete yet.

it would make sense to define a header fileset on CMake >= 3.23 for the generated headers.

we should do is set a property on ${cxx_target} which tells where it is generated headers are placed

Something like these solutions will be great.

It will be great if we can come up with a canonical solution in corrosion and add some documentation for this kind of usage so that users can simply follow the instructions to do it. CMake is already too hard for many people (at least for me :() and any guess during the process will make this process much harder

niyue commented 1 year ago

There is another installation related issue which I think might be related with this issue (let me know if I need to open a new issue for it). If I try to use my_lib (see above) in another program my_app, during linking my_app, linker (ld under macOS 12) will report error like:

ld: library not found for -lmy_cxx_bridge_target-static

Here my_cxx_bridge_target is the target I specified when calling corrision_add_cxxbridge function, and it seems my_cxx_bridge_target-static is an internal target introduced by corrosion within this function, and this my_cxx_bridge_target-static target is marked as INTERFACE_LINK_LIBRARIES for the my_cxx_bridge_target (I think it is this line https://github.com/corrosion-rs/corrosion/blob/b921fd09155c05255a6b0df6d9901f919ce81b6b/cmake/Corrosion.cmake#L605). So when linking against my_cxx_bridge_target, the my_cxx_bridge_target-static will be linked. However, this target may require export/install to be linked.

I am not completely sure what the best approach for addressing this issue, do you think if this should be addressed in corrosion or if this should be done in corrosion users' side? If it should be done by corrosion users, what could be the recommended way to do it? Thanks.

jschwe commented 1 year ago

The reason why I think we should not add any install rules into corrision_add_cxxbridge is that we don't not if user want to export this headers. The headers could be internal for the project.

There probably was a misunderstanding here - I was not proposing to add any install rules directly from corrosion. AS you said this should be up to the user.

@jschwe I think what we should do is set a property on ${cxx_target} which tells where it is generated headers are placed.

In CMake >= 3.23 this is what the HEADERS FileSet type is intended for and corrosion could create such a fileset and add the headers via target_sources( target PUBLIC FILESET corrosion_<target>_header HEADERS) . CMake would then automatically set the target properties HEADER_SETS, HEADER_DIRS and update INCLUDE_DIRECTORIES (among others).

For older CMake versions providing a property to read is one solution, but I guess we could also just document where the files are generated too, since the path is pretty simple and unlikely to change.

It will be great if we can come up with a canonical solution in corrosion and add some documentation for this kind of usage so that users can simply follow the instructions to do it.

Definitely - the documentation of corrosion can certainly be improved greatly. I've been wanting to add an mdbook style documentation, so we don't have everything in one long readme and can add more examples without bloating the readme, but haven't had the time.

There is another installation related issue which I think might be related with this issue

Please open a new issue for that. It's probably not related to the new cxxbridge integration but rather an existing problem that hasn't surfaced because there are no tests using install inside a project and apparently not so many users either.