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

[giflib] Add export CMake #26869

Closed
wants to merge 12 commits into from
Closed

[giflib] Add export CMake #26869

wants to merge 12 commits into from

Conversation

LilyWangLL
Copy link
Contributor

Describe the pull request

@LilyWangLL LilyWangLL added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Sep 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Sep 19, 2022
View reviewed changes
dg0yt
dg0yt suggested changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Wrong approach to fix #26833.
Another unofficial config doesn't resolve the problem of CMake's FindGIF.cmake needing help, i.e. needing a wrapper.
The config is not used by any downstream port without patching.

github-actions[bot]
github-actions bot previously approved these changes Sep 20, 2022
View reviewed changes
github-actions[bot]
github-actions bot previously approved these changes Sep 20, 2022
View reviewed changes
dg0yt
dg0yt reviewed Sep 21, 2022
View reviewed changes
ports/giflib/vcpkg-cmake-wrapper.cmake Outdated
Comment on lines 1 to 12
_find_package(unofficial-giflib CONFIG REQUIRED)
add_library(GIF::GIF ALIAS unofficial::giflib::gif)

get_filename_component(GIF_INCLUDE_DIRS "${CMAKE_CURRENT_LIST_DIR}" PATH)
get_filename_component(GIF_INCLUDE_DIRS "${LUA_INCLUDE_DIR}" PATH)

set(GIF_FOUND TRUE)
set(GIF_INCLUDE_DIRS ${_IMPORT_PREFIX}/include)
set(GIF_LIBRARIES unofficial::giflib::gif)
set(GIF_VERSION @GIFLIB_VERSION@)

unset(_IMPORT_PREFIX)
Copy link
Contributor

Choose a reason for hiding this comment

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

So many error in a short file!

And I repeat that you don't need exported config. Cf. zlib.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dg0yt I think this should be okay. what are you worried about?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really hard...

dg0yt
dg0yt suggested changes Sep 21, 2022
View reviewed changes
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

And again, do not implement this with imported config. It is just a simple lib without dependencies.

ports/giflib/vcpkg-cmake-wrapper.cmake Outdated
@@ -0,0 +1,12 @@
_find_package(unofficial-giflib CONFIG REQUIRED)
add_library(GIF::GIF ALIAS unofficial::giflib::gif)
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be guarded by if(NOT TARGET GIF::GIF).

You must not use ALIAS here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This must be guarded by if(NOT TARGET GIF::GIF).

Agreed.

You must not use ALIAS here.

Why?
Our goal is to be fully compatible with FindGIF.cmake, so we should provide all the targets and macros that cmake provides in FindGIF.

Copy link
Contributor

@dg0yt dg0yt Sep 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ALIAS is for internal use in a CMake project. But this is exported config. You can use INTERFACE libraries. Given the vendored CMakeLists.txt, you can just directly export GIF::GIF.

Copy link
Contributor

Choose a reason for hiding this comment

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

ALIAS is for internal use in a CMake project. But this is exported config. You can use INTERFACE libraries. Given the vendored CMakeLists.txt, you can just directly export GIF::GIF.

I think not, GIF::GIF is provided by cmake, not by giflib officially.

Copy link
Contributor

Choose a reason for hiding this comment

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

FindGIF.cmake from CMake is defining the official interface of find_package(GIF). So yes, just export GIF::GIF directly, "to be fully compatible with FindGIF.cmake".
Or don't export anything, and use plain find_library and select_library_configurations, as we do in other ports.

Copy link
Contributor

Choose a reason for hiding this comment

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

You must not use ALIAS here.

Why?

And CMake 3.7.2 even doesn't allow this alias:

CMake Error at /Users/vagrant/Data/work/1/s/scripts/buildsystems/vcpkg.cmake:618 (_add_library):
  add_library cannot create ALIAS target "GIF::GIF" because target
  "unofficial::giflib::gif" is IMPORTED.

Hint: You can build test port cmake-user locally before pushing the next update.

ports/giflib/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved
ports/giflib/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved
ports/giflib/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Sep 21, 2022
View reviewed changes
github-actions[bot]
github-actions bot previously approved these changes Sep 21, 2022
View reviewed changes
dg0yt
dg0yt reviewed Sep 22, 2022
View reviewed changes
ports/giflib/CMakeLists.txt
@@ -26,9 +26,10 @@ if(HAVE_REALLOCARRAY)
add_definitions(-DHAVE_REALLOCARRAY)
endif()

add_library(gif ${GIFLIB_SOURCES})
add_library(GIF ${GIFLIB_SOURCES})
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to set the OUTPUT_NAME to lower-case gif now. Then x64-linux will be happy.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 25, 2022

Why not to set GIF_LIBRARIES to GIF::GIF:
This creates problems (i.e. requires patching) in export of downstream CMake config. The regular value of GIF_LIBRARIES can be exported directly, but GIF::GIF needs find_dependency(GIF).
So please use the select_library_configurations pattern, as we do in zlib etc.

@Chlumsky
Copy link
Contributor

Any chance this might be completed?

@LilyWangLL
Copy link
Contributor Author

Any chance this might be completed?

I will continue to fix this in the next week.

@JackBoosY JackBoosY assigned Cheney-W and unassigned JackBoosY Oct 14, 2022
@dg0yt dg0yt mentioned this pull request Oct 21, 2022
Merged
@LilyWangLL
Copy link
Contributor Author

Closed as duplicate of #27369.

@LilyWangLL LilyWangLL closed this Oct 27, 2022
@LilyWangLL LilyWangLL deleted the dev/LilyWang/issue26833 branch December 19, 2022 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt requested changes

@JackBoosY JackBoosY JackBoosY left review comments

@github-actions github-actions[bot] github-actions[bot] approved these changes

Assignees

@Cheney-W Cheney-W

Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[giflib] Debug library linked in release configuration
5 participants
@LilyWangLL @dg0yt @Chlumsky @JackBoosY @Cheney-W