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

Add parameter to corrosion_import_crate to return list of added targets. #218

Closed
jschwe opened this issue Sep 19, 2022 · 7 comments
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@jschwe
Copy link
Collaborator

jschwe commented Sep 19, 2022

This could be used by the caller to set some properties for all (or a filtered subset) of targets.
You can already manually set the properties, but in cases where there are a lot of targets, it may be more convenient to just iterate over a list that corrosion provides.

@jschwe jschwe added the enhancement New feature or request label Sep 19, 2022
@temeddix
Copy link
Contributor

Yeah, this is definitely needed in actual developement.

@jschwe
Copy link
Collaborator Author

jschwe commented Jan 25, 2023

this is definitely needed in actual developement.

I'm curious what your usecase is that you "definitely need" this. From my point of view it's just a minor convenience improvement. Could you elaborate?

@temeddix
Copy link
Contributor

temeddix commented Jan 25, 2023

We have a situation where we have to compile multiple crates inside a workspace crate. Since our repository is public, it is https://github.com/cunarist/app-template. It's a template for app development written in Flutter and Rust

We have a workspace rust crate native and some other library crates inside it, like this:

  • native: workspace crate
    • first_crate: library crate
    • second_crate: library crate
    • ...(More library crates will be added in the future)

With this situation, we can compile multiple library crates at once with a CMAKE file like this:

# Part of CMAKE file
corrosion_import_crate(MANIFEST_PATH ../native/Cargo.toml)
set(CRATE_NAME "first_crate")
target_link_libraries(${BINARY_NAME} PRIVATE ${CRATE_NAME})\
set(CRATE_NAME "second_crate")
target_link_libraries(${BINARY_NAME} PRIVATE ${CRATE_NAME})

However, if we add new library crate inside the native workspace crate, we have to modify this CMAKE file to have new line of target_link_libraries. Therefore it would be nice to have something like this:

# Part of CMAKE file
corrosion_import_crate(MANIFEST_PATH ../native/Cargo.toml)
foreach(LIB ${LIBS}) # Assuming that a list name LIBS is from corrosion_import_crate
target_link_libraries(${BINARY_NAME} PRIVATE ${LIB})
endforeach()

@jschwe
Copy link
Collaborator Author

jschwe commented Jan 25, 2023

I'm currently a bit tight on time, but if you would be interested in opening a PR, this feature would be quite simple to implement.

We already save the information here in created_targets, so all that has to be done is propogate this variable back up the callchain to corrosion_import_crate and add a parameter to corrosion_import_crate for the user to specify a variable name, so corrosion_import_crate can use set(${var_name} "${imported_crates}" PARENT_SCOPE) to propogate this back to the user.

@jschwe jschwe self-assigned this Jan 25, 2023
@jschwe
Copy link
Collaborator Author

jschwe commented Jan 25, 2023

Actually things happened, and this issue has moved up on my priority list. I'll probably be able to work on this at the end of the week.

@temeddix
Copy link
Contributor

temeddix commented Jan 25, 2023

I did make a pull request as you described. Please tell me if there is any problem.
P.S: I didn't see that you mentioned about your priority😗. Well, I already made a PR request anyway...

@jschwe
Copy link
Collaborator Author

jschwe commented Jan 25, 2023

P.S: I didn't see that you mentioned about your priority😗. Well, I already made a PR request anyway...

I didn't have the time yet, so definitely thanks for doing this! Closing, since #312 is merged now.

@jschwe jschwe closed this as completed Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants