Upgrade to Corrosion v0.4.10 #2220
Conversation
wantehchang
force-pushed
the
fix-windows-build-2
branch
2 times, most recently
from
8d93ef0
to
d91b39c
Compare
We need the bug fix corrosion-rs/corrosion#511.
wantehchang
force-pushed
the
fix-windows-build-2
branch
from
d91b39c
to
3875db6
Compare
wantehchang
changed the title
|
@fdintino Please take a look. Thanks! |
| add_library(rav1e::rav1e STATIC IMPORTED) | ||
| add_dependencies(rav1e::rav1e rav1e) | ||
| target_link_libraries(rav1e::rav1e INTERFACE "${RAV1E_LIBRARIES}") | ||
| target_link_libraries(rav1e::rav1e INTERFACE "${Rust_CARGO_TARGET_LINK_NATIVE_LIBS}") | ||
| target_link_options(rav1e::rav1e INTERFACE "${Rust_CARGO_TARGET_LINK_OPTIONS}") |
There was a problem hiding this comment.
I am not sure if we should use INTERFACE or PRIVATE here.
There was a problem hiding this comment.
INTERFACE is the way to go because we are creating a library from scratch. Setting PRIVATE dependencies would not change anything as it is a shell library. INTERFACE ensures anybody depending on rav1e:rav1e will also use the dep.
There was a problem hiding this comment.
Since Rust_CARGO_TARGET_LINK_OPTIONS is an empty list on non-Windows platforms, I wonder if we should guard the target_link_options() call in an if statement:
if (Rust_CARGO_TARGET_LINK_OPTIONS)
target_link_options(rav1e::rav1e INTERFACE "${Rust_CARGO_TARGET_LINK_OPTIONS}")
endif()
I am also not sure if we need to quote ${Rust_CARGO_TARGET_LINK_NATIVE_LIBS} and ${Rust_CARGO_TARGET_LINK_OPTIONS} in the target_link_libraries() and target_link_options() calls.
There was a problem hiding this comment.
Since
Rust_CARGO_TARGET_LINK_OPTIONSis an empty list on non-Windows platforms, I wonder if we should guard thetarget_link_options()call in an if statement:if (Rust_CARGO_TARGET_LINK_OPTIONS) target_link_options(rav1e::rav1e INTERFACE "${Rust_CARGO_TARGET_LINK_OPTIONS}") endif()
That seems fine.
I am also not sure if we need to quote
${Rust_CARGO_TARGET_LINK_NATIVE_LIBS}and${Rust_CARGO_TARGET_LINK_OPTIONS}in thetarget_link_libraries()andtarget_link_options()calls.
Probably not given it would be a single item or ';' separated list. Though I think quoting is usually safest.
There was a problem hiding this comment.
Thanks for the reply. I did some experiments and found that both of the following are harmless:
add_library(wtc wtc.c)
target_link_options(wtc INTERFACE)
target_link_options(wtc INTERFACE "")
So we can just call target_link_options() unconditionally here.
|
Nice finding and solution ! |
| add_library(rav1e::rav1e STATIC IMPORTED) | ||
| add_dependencies(rav1e::rav1e rav1e) | ||
| target_link_libraries(rav1e::rav1e INTERFACE "${RAV1E_LIBRARIES}") | ||
| target_link_libraries(rav1e::rav1e INTERFACE "${Rust_CARGO_TARGET_LINK_NATIVE_LIBS}") | ||
| target_link_options(rav1e::rav1e INTERFACE "${Rust_CARGO_TARGET_LINK_OPTIONS}") |
There was a problem hiding this comment.
Since Rust_CARGO_TARGET_LINK_OPTIONS is an empty list on non-Windows platforms, I wonder if we should guard the target_link_options() call in an if statement:
if (Rust_CARGO_TARGET_LINK_OPTIONS)
target_link_options(rav1e::rav1e INTERFACE "${Rust_CARGO_TARGET_LINK_OPTIONS}")
endif()
I am also not sure if we need to quote ${Rust_CARGO_TARGET_LINK_NATIVE_LIBS} and ${Rust_CARGO_TARGET_LINK_OPTIONS} in the target_link_libraries() and target_link_options() calls.
| @@ -1,5 +1,5 @@ | |||
| set(AVIF_LOCAL_RAV1E_GIT_TAG v0.7.1) | |||
| set(AVIF_LOCAL_CORROSION_GIT_TAG v0.4.4) | |||
| set(AVIF_LOCAL_CORROSION_GIT_TAG v0.4.10) | |||
There was a problem hiding this comment.
Note that I know Corrosion v0.5.0 is available. The reason I did not upgrade to v0.5.0 in this pull request is that I didn't spend the time understanding the breaking change mentioned in the release notes. We can upgrade to v0.5.0 in a follow-up pull request.
We need the bug fix corrosion-rs/corrosion#511.