[giflib] Add export CMake by LilyWangLL · Pull Request #26869 · microsoft/vcpkg

LilyWangLL · 2022-09-19T08:59:15Z

Describe the pull request

What does your PR fix?
Fixes [giflib] Debug library linked in release configuration #26833, add export CMake and fix Release mode depends on Debug lib.
Usage have test pass.

dg0yt

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.

dg0yt · 2022-09-21T05:04:58Z

ports/giflib/vcpkg-cmake-wrapper.cmake

+_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)


So many error in a short file!

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

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

This is really hard...

dg0yt

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

dg0yt · 2022-09-21T07:08:06Z

ports/giflib/vcpkg-cmake-wrapper.cmake

@@ -0,0 +1,12 @@
+_find_package(unofficial-giflib CONFIG REQUIRED)
+add_library(GIF::GIF ALIAS unofficial::giflib::gif)


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

You must not use ALIAS here.

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.

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.

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.

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.

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

dg0yt · 2022-09-22T10:54:00Z

ports/giflib/CMakeLists.txt

 endif()

-add_library(gif ${GIFLIB_SOURCES})
+add_library(GIF ${GIFLIB_SOURCES})


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

dg0yt · 2022-09-25T16:01:19Z

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 · 2022-10-12T10:13:37Z

Any chance this might be completed?

LilyWangLL · 2022-10-13T01:55:02Z

Any chance this might be completed?

I will continue to fix this in the next week.

LilyWangLL · 2022-10-27T02:50:25Z

Closed as duplicate of #27369.

LilyWangLL added 2 commits September 19, 2022 01:55

[giflib] Add export CMake

cdfb8ef

x-add-version

6a0f30d

LilyWangLL added category:port-bug The issue is with a library, which is something the port should already support info:internal labels Sep 19, 2022

LilyWangLL assigned JackBoosY Sep 19, 2022

github-actions bot previously approved these changes Sep 19, 2022

View reviewed changes

dg0yt suggested changes Sep 19, 2022

View reviewed changes

LilyWangLL added 2 commits September 20, 2022 00:58

Add vcpkg-cmake-wrapper.cmake and usage

c990127

x-add-version

5e7c9fc

LilyWangLL dismissed github-actions[bot]’s stale review via 5e7c9fc September 20, 2022 07:59

github-actions bot previously approved these changes Sep 20, 2022

View reviewed changes

LilyWangLL added 2 commits September 20, 2022 01:11

update vcpkg.json

528a909

x-add-version

771c2ce

LilyWangLL dismissed github-actions[bot]’s stale review via 771c2ce September 20, 2022 08:12

github-actions bot previously approved these changes Sep 20, 2022

View reviewed changes

dg0yt reviewed Sep 21, 2022

View reviewed changes

dg0yt suggested changes Sep 21, 2022

View reviewed changes

LilyWangLL added 2 commits September 21, 2022 00:13

Modify vcpkg-cmake-wrapper.cmake

f4aeaf0

x-add-version

dbca479

LilyWangLL dismissed github-actions[bot]’s stale review via dbca479 September 21, 2022 07:14

github-actions bot previously approved these changes Sep 21, 2022

View reviewed changes

dg0yt reviewed Sep 21, 2022

View reviewed changes

ports/giflib/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved

LilyWangLL added 2 commits September 21, 2022 00:39

Modify vcpkg-cmake-wrapper.cmake

4b2bdc5

x-add-version

6837cb8

LilyWangLL dismissed github-actions[bot]’s stale review via 6837cb8 September 21, 2022 07:40

github-actions bot previously approved these changes Sep 21, 2022

View reviewed changes

LilyWangLL added 2 commits September 21, 2022 23:54

Change target name

cae36cb

x-add-version

9543f1b

LilyWangLL dismissed github-actions[bot]’s stale review via 9543f1b September 22, 2022 06:55

github-actions bot approved these changes Sep 22, 2022

View reviewed changes

dg0yt reviewed Sep 22, 2022

View reviewed changes

JackBoosY assigned Cheney-W and unassigned JackBoosY Oct 14, 2022

dg0yt mentioned this pull request Oct 21, 2022

[giflib] Fix multi-config find_package(GIF) #27369

Merged

LilyWangLL closed this Oct 27, 2022

LilyWangLL deleted the dev/LilyWang/issue26833 branch December 19, 2022 09:11

		@@ -0,0 +1,12 @@
		_find_package(unofficial-giflib CONFIG REQUIRED)
		add_library(GIF::GIF ALIAS unofficial::giflib::gif)

Conversation

LilyWangLL commented Sep 19, 2022

What does your PR fix?

Uh oh!

dg0yt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg0yt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg0yt Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg0yt commented Sep 25, 2022

Uh oh!

Chlumsky commented Oct 12, 2022

Uh oh!

LilyWangLL commented Oct 13, 2022

Uh oh!

LilyWangLL commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dg0yt Sep 21, 2022 •

edited

Loading