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

[obsgolem]

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]

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]

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]

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]

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]

string(MAKE_C_IDENTIFIER)

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

[jschwe]

Yes, please

[obsgolem]

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]

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.

[jschwe]

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/cmake/Corrosion.cmake

Line 938 in 4a134df

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

[jschwe]

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?

[obsgolem]

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

[obsgolem]

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]

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