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.

Sign up for GitHub

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

Jump to bottom

Upgrade to Corrosion v0.4.10 #2220

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Upgrade to Corrosion v0.4.10 #2220

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions cmake/Modules/LocalRav1e.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

set(AVIF_LOCAL_CARGOC_GIT_TAG v0.9.27)

set(RAV1E_LIB_FILENAME
Expand Down Expand Up @@ -97,19 +97,10 @@ else()
file(MAKE_DIRECTORY ${RAV1E_INCLUDE_DIR})
set(RAV1E_FOUND ON)

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.
list(TRANSFORM RAV1E_LIBRARIES REPLACE "^/" "-")
# Remove msvcrt from RAV1E_LIBRARIES since it's linked by default
list(REMOVE_ITEM RAV1E_LIBRARIES "msvcrt.lib" "-lmsvcrt")
endif()

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.

set_target_properties(rav1e::rav1e PROPERTIES IMPORTED_LOCATION "${RAV1E_LIB_FILENAME}" AVIF_LOCAL ON FOLDER "ext/rav1e")
target_include_directories(rav1e::rav1e INTERFACE "${RAV1E_INCLUDE_DIR}")

Expand Down
Loading