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

Upgrade to Corrosion v0.4.10 #2220

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

wantehchang
Copy link
Collaborator

@wantehchang wantehchang commented Jun 21, 2024

We need the bug fix corrosion-rs/corrosion#511.

@wantehchang wantehchang marked this pull request as draft June 21, 2024 00:29
@wantehchang wantehchang force-pushed the fix-windows-build-2 branch 2 times, most recently from 8d93ef0 to d91b39c Compare June 21, 2024 01:55
@wantehchang wantehchang changed the title Pass /defaultlib:msvcrt to target_link_options() Upgrade to Corrosion v0.4.10 Jun 21, 2024
@wantehchang wantehchang requested a review from jzern June 21, 2024 03:33
@wantehchang wantehchang marked this pull request as ready for review June 21, 2024 03:33
@wantehchang
Copy link
Collaborator Author

@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}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we should use INTERFACE or PRIVATE here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

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 the target_link_libraries() and target_link_options() calls.

Probably not given it would be a single item or ';' separated list. Though I think quoting is usually safest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vrabaud
Copy link
Collaborator

vrabaud commented Jun 21, 2024

Nice finding and solution !

Copy link
Collaborator Author

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both for the review!

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}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@wantehchang wantehchang merged commit 8130965 into AOMediaCodec:main Jun 21, 2024
28 checks passed
@wantehchang wantehchang deleted the fix-windows-build-2 branch June 21, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants