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

Generated headers and CMake install #261

Open
jschwe opened this issue Nov 8, 2022 · 6 comments
Open

Generated headers and CMake install #261

jschwe opened this issue Nov 8, 2022 · 6 comments

Comments

@jschwe
Copy link
Collaborator

jschwe commented Nov 8, 2022

    @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 #244 (comment)

@jschwe
Copy link
Collaborator Author

jschwe commented Nov 8, 2022

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

jschwe commented Nov 8, 2022

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

@msvetkin
Copy link
Contributor

msvetkin commented Nov 8, 2022

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

niyue commented Nov 8, 2022

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

  • In my case, I am okay with CMake >= 3.23 and I did some web search previously and it seems header fileset is the latest CMake solution for such problem
  • either having some target property or allowing users to specify a directory for header generation can help I think

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

niyue commented Nov 8, 2022

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

target_link_libraries(${target_name} INTERFACE ${target_name}-static)
). 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
Copy link
Collaborator Author

jschwe commented Nov 9, 2022

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.

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

No branches or pull requests

3 participants