Windows: change leading / to - in RAV1E_LIBRARIES #2219
Conversation
Fix the ci-windows.yml error in the "Build libavif (ninja)" step: ninja: error: '/defaultlib:msvcrt', needed by 'avif_example_decode_memory.exe', missing and no known rule to make it RAV1E_LIBRARIES is passed to target_link_libraries(). Item names starting with -, but not -l or -framework, are treated by target_link_libraries() as linker flags. On Windows, linker flags may start with / (e.g., /defaultlib:msvcrt), so we need to change / to - before passing the linker flags to target_link_libraries(). Otherwise target_link_libraries() will incorrectly treat those linker flags as libraries.
| @@ -99,6 +99,10 @@ else() | |||
|
|
|||
| set(RAV1E_LIBRARIES ${Rust_CARGO_TARGET_LINK_NATIVE_LIBS}) | |||
| if(WIN32) | |||
| # If an item starts with "/" (e.g., "/defaultlib:msvcrt"), change the | |||
| # leading "/" to "-" so that the target_link_libraries() call below | |||
| # will treat the item as a linker flag. | |||
There was a problem hiding this comment.
See https://cmake.org/cmake/help/latest/command/target_link_libraries.html#overview
- A link flag: Item names starting with -, but not -l or -framework, are treated as linker flags. ...
There was a problem hiding this comment.
I wonder if we should file a cmake bug report about this issue. On Windows, linker flags may start with /. Obviously a concern is that an absolute pathname of a library also starts with / if the drive letter is omitted.
Or we can ask Rust to use -defaultlib:msvcrt instead of /defaultlib:msvcrt.
There was a problem hiding this comment.
It sounds like in newer versions of cmake this flag would be better placed in LINK_OPTIONS. It looks like -nodefaultlibs is used elsewhere in the rust codebase, so maybe this was just an oversight.
There was a problem hiding this comment.
Yes, I think we can replace this fix with the following:
set(RAV1E_LINK_OPTIONS ${RAV1E_LIBRARIES})
list(FILTER RAV1E_LIBRARIES EXCLUDE REGEX "/.*")
list(FILTER RAV1E_LINK_OPTIONS INCLUDE REGEX "/.*")
target_link_options(rav1e::rav1e INTERFACE "${RAV1E_LINK_OPTIONS}")
Let me merge this fix first, and then experiment with the alternative fix.
| # If an item starts with "/" (e.g., "/defaultlib:msvcrt"), change the | ||
| # leading "/" to "-" so that the target_link_libraries() call below | ||
| # will treat the item as a linker flag. | ||
| list(TRANSFORM RAV1E_LIBRARIES REPLACE "^/" "-") |
There was a problem hiding this comment.
The list(TRANSFORM) operation is new in cmake 3.12:
https://cmake.org/cmake/help/latest/command/list.html#transform
Our cmake minimum required version is 3.13, so it is safe to use list(TRANSFORM).
| @@ -99,6 +99,10 @@ else() | |||
|
|
|||
| set(RAV1E_LIBRARIES ${Rust_CARGO_TARGET_LINK_NATIVE_LIBS}) | |||
| if(WIN32) | |||
| # If an item starts with "/" (e.g., "/defaultlib:msvcrt"), change the | |||
| # leading "/" to "-" so that the target_link_libraries() call below | |||
| # will treat the item as a linker flag. | |||
There was a problem hiding this comment.
It sounds like in newer versions of cmake this flag would be better placed in LINK_OPTIONS. It looks like -nodefaultlibs is used elsewhere in the rust codebase, so maybe this was just an oversight.
| @@ -99,6 +99,10 @@ else() | |||
|
|
|||
| set(RAV1E_LIBRARIES ${Rust_CARGO_TARGET_LINK_NATIVE_LIBS}) | |||
There was a problem hiding this comment.
@fdintino Frankie: I can't find documentation for the Rust_CARGO_TARGET_LINK_NATIVE_LIBS variable. Is it a public variabe that we can use? Or is it for internal use by Rust Cargo?
There was a problem hiding this comment.
I found this is a bug in Corrosion that is fixed in v0.4.10:
https://github.com/corrosion-rs/corrosion/releases/tag/v0.4.10
corrosion-rs/corrosion#511
Fix the ci-windows.yml error in the "Build libavif (ninja)" step:
ninja: error: '/defaultlib:msvcrt', needed by
'avif_example_decode_memory.exe', missing and no known rule to make it
RAV1E_LIBRARIES is passed to target_link_libraries(). Item names starting with -, but not -l or -framework, are treated by target_link_libraries() as linker flags. On Windows, linker flags may start with / (e.g., /defaultlib:msvcrt), so we need to change / to - before passing the linker flags to target_link_libraries(). Otherwise target_link_libraries() will incorrectly treat those linker flags as libraries.