Fix Integrator build script #139
Conversation
|
Can there be a test? Will there not be problems if the directories contains spaces? Or is that properly escaped. (Maybe that's another thing worth testing: spaces or special characters in the source and build paths) |
I haven't tested, but I would imagine that to be problematic. I think there are probably also a couple of other places in our code that could be problematic for paths with spaces (but I'll look into that separately). So that means I'll definitely also have to add a test. |
|
I finally got back to this. I chose |
cmake/Corrosion.cmake
Outdated
| @@ -105,8 +105,13 @@ function(_add_cargo_build) | |||
| set (build_dir .) | |||
| endif() | |||
|
|
|||
| set(link_libs "$<GENEX_EVAL:$<TARGET_PROPERTY:cargo-build_${target_name},CARGO_LINK_LIBRARIES>>") | |||
| set(search_dirs "$<GENEX_EVAL:$<TARGET_PROPERTY:cargo-build_${target_name},CARGO_LINK_DIRECTORIES>>") | |||
| # Join the list with `|` as a custom delimiter, since the `;` is expanded when passed | |||
There was a problem hiding this comment.
Did you mean : here?
Path can contain : on Linux iirc.
There was a problem hiding this comment.
Yes, I changed to :, actually because PATH is : delimited. So using the same delimiter for our usecase seems reasonable. I adjusted the wording to make it clearer that file/directory paths cannot contain :.
| @@ -0,0 +1,10 @@ | |||
| // Check that libraries located at a path containing a space can also be linked. | |||
There was a problem hiding this comment.
I'd go with a even more complex path for the test. Include a few symbol, some emoji, an such.
There was a problem hiding this comment.
I added some chinese characters and emojis. I don't really want to add any of the ascii special characters, because then we would have to check what the common subset of supported special characters in filepath is across our supported operating systems and generators/compilers.
There was a problem hiding this comment.
Looks like ninja 1.11 (which is not even out yet) would be required to support non-ansi filepaths on windows. See ninja-build/ninja#1915 for details.
Use a custom separator to prevent list expansion. Library paths may contain spaces so we need to use a custom delimiter. The `:` should work well as a custom delimiter, since it can't be part of a valid filepath on any platform I know. Closes corrosion-rs#128 Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
We use
COMMAND_EXPAND_LISTS, which also expands lists in generatorexpressions. Therefore the values of the env variables will be space
seperated.
Closes #128