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

Added ability to import a crate containing lib and bin targets with the same name #394

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

obsgolem
Copy link

@obsgolem obsgolem commented May 26, 2023

We do this by adding the ability to put the bin and lib targets into separate CMake namespaces. This also takes some initial steps towards breaking the connection between Rust target names and CMake target names. This structure could be useful for #58 eventually

I currently use __ when :: is not valid in a target name. This can be easily changed if something else is desirable. This feature is entirely invisible unless you opt into it.

@jschwe
Copy link
Collaborator

jschwe commented May 26, 2023

I probably won't have time to review this until one week from now.
I'd also be interested if you have a specific usecase where you really need this feature and can't just edit the cargo.toml to get distinct names. This MR does complicate the code and create a maintenance burden, so I'm a bit hesitant.

@obsgolem
Copy link
Author

So, a decent chunk of this code is just plumbing that would be needed for #58. In fact, I could do up a follow up that implements that issue using a very small amount of code. Since it is shared work, my opinion is that the maintenance burden isn't too bad.

Personally, I could rename my targets in my Rust project, but it really doesn't make sense to. If I have a binary named x I should be able to refer to the library code as use x::whatever instead of use libx::whatever.

@obsgolem
Copy link
Author

One question: is IMPORTED_CRATES intended to yield a list of targets instead of crates? If so, would it be reasonable to add a new IMPORTED_TARGETS input and deprecate IMPORTED_CRATES?

@jschwe
Copy link
Collaborator

jschwe commented May 29, 2023

is IMPORTED_CRATES intended to yield a list of targets instead of crates

Well, previously the crate name was the same as the target name (a single rust Cargo.toml package manifest contains zero or one library crates, and 0-n bin crates).

I'm conservative regarding deprecating / renaming things - I think it should be sufficient to document that the variable will contain a list of CMake targets, representing the crates that were imported.
I am considering splitting corrosion_import_crate into corrosion_import_workspace and corrosion_import_package, and if I ever do that, then the parameter names for the new functions could also be adjusted.

I currently use __ when :: is not valid in a target name.

I would document that the transformation is string(MAKE_C_IDENTIFIER), which effectively does the same.

@obsgolem
Copy link
Author

obsgolem commented May 29, 2023

string(MAKE_C_IDENTIFIER)

Just to be clear, do you want me to replace my existing string(REPLACE) with this?

@jschwe
Copy link
Collaborator

jschwe commented May 30, 2023

Yes, please

@obsgolem
Copy link
Author

Are you sure this has the correct semantics? It converts e.g. flags-lib into flags_lib. This is currently causing build failures; I can work around it, but personally I prefer the more targeted string(REPLACE).

@jschwe
Copy link
Collaborator

jschwe commented Jun 1, 2023

It converts e.g. flags-lib into flags_lib

Ah, I see. No that is not intended. In that case lets stick with the string(REPLACE). Sorry for confusion.

Copy link
Collaborator

@jschwe jschwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good besides the two comments.
Also it would be good if you could document the two new flags, by adding it to the documentation comment here:

corrosion_import_crate(

The documentation will automatically be included into the usage documentation here: https://corrosion-rs.github.io/corrosion/usage.html

Edit: Also my apologies for the long delay until reviewing

@@ -111,6 +119,7 @@ function(_generator_add_package_targets)
_corrosion_add_library_target(
WORKSPACE_MANIFEST_PATH "${workspace_manifest_path}"
TARGET_NAME "${target_name}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering: Is the original target_name even still used after we defined target_name_cmake? Is there a reason why we would need both target_name and target_name_cmake?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is used to form the lib file names.

cmake/Corrosion.cmake Outdated Show resolved Hide resolved
@obsgolem
Copy link
Author

obsgolem commented Oct 7, 2024

Whoo boy, sorry for not getting back to this for so long. I redid the entire MR since it was so far behind and I had forgotten exactly what I had done in the first place. I can close this and open up a new MR if you would prefer.

@jschwe
Copy link
Collaborator

jschwe commented Oct 8, 2024

I'll be travelling a lot in the next 3 weeks, so it might take a while till I can review this PR (again).

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