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

[curl] Revert revert of -imp suffix removal. #6698

Merged
merged 3 commits into from
Jun 26, 2019
+28 −110
Merged

[curl] Revert revert of -imp suffix removal. #6698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ee6a60
[curl] Revert revert of `-imp` suffix removal.
ras0219-msft May 29, 2019
46ac560
[idevicerestore][libideviceactivation] Simplify dependency on curl
ras0219-msft Jun 4, 2019
8da0405
[curl] Trivial change to provoke rebuild
ras0219-msft Jun 10, 2019
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
13 changes: 13 additions & 0 deletions ports/curl/0005_remove_imp_suffix.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index eca9a8a..6f72955 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -97,7 +97,7 @@ endif()
set_target_properties(${LIB_NAME} PROPERTIES PREFIX "")
set_target_properties(${LIB_NAME} PROPERTIES IMPORT_PREFIX "")

-if(WIN32)
+if(WIN32 AND 0)
if(BUILD_SHARED_LIBS)
# Add "_imp" as a suffix before the extension to avoid conflicting with the statically linked "libcurl.lib"
set_target_properties(${LIB_NAME} PROPERTIES IMPORT_SUFFIX "_imp.lib")
2 changes: 1 addition & 1 deletion ports/curl/CONTROL
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Source: curl
Version: 7.65.0-1
Version: 7.65.0-2
Build-Depends: zlib
Description: A library for transferring data with URLs
Default-Features: ssl
Expand Down
11 changes: 9 additions & 2 deletions ports/curl/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ vcpkg_from_github(
0002_fix_uwp.patch
0003_fix_libraries.patch
0004_nghttp2_staticlib.patch
0005_remove_imp_suffix.patch
)

string(COMPARE EQUAL "${VCPKG_LIBRARY_LINKAGE}" "static" CURL_STATICLIB)
Expand Down Expand Up @@ -140,17 +141,23 @@ if(EXISTS "${CURRENT_PACKAGES_DIR}/bin/curl${EXECUTABLE_SUFFIX}")
endif()
endif()

if(EXISTS ${CURRENT_PACKAGES_DIR}/lib/libcurl_imp.lib OR EXISTS ${CURRENT_PACKAGES_DIR}/debug/lib/libcurl-d_imp.lib)
# Because vcpkg only ever installs one or the other, there is no need to have the libraries be different names.
# Using the same name improves compatibility, because downstream projects can always rely on "libcurl.lib"
Copy link
Contributor

Choose a reason for hiding this comment

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

because downstream projects can always rely on "libcurl.lib"

Hmm. What about libcurl-d? Or are you patching downstream non-cmake projects to use the correct configuration?

Someone using VCPKG introduced that suffix into CMake

find_library(CURL_LIBRARY_DEBUG NAMES
  # Windows MSVC CMake builds in debug configuration on vcpkg:
    libcurl-d_imp
    libcurl-d
    HINTS ${PC_CURL_LIBRARY_DIRS}
)

If you are planning to drop it in VCPKG please also fix CMakes find module at the same time.

How do you plan to handle those cases in the future?
I mean you are applying a patch to a library which does not require it and works perfectly fine without it. Thus the patch will probably never be merged upstream. It is the downstream packages which assume the wrong library name and should thus be patched/improved to account for the correct name (although that is probably a lot of work.). So what I am asking is basically some better defined guidelines from the team on how to treat these cases.

From the current guidlines:

Avoid functional patches. Patches should be considered a last resort to implement compatibility when there's no other way.
When patches can't be avoided, do not modify the default behavior. The ideal lifecycle of a patch is to get merged upstream and no longer be needed. Try to keep this goal in mind when deciding how to patch something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have written "libcurl.lib or libcurl-d.lib based on configuration". My change doesn't remove the -d suffix :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok glad to here that there is no plan to remove the debug suffix.

Just and idea of mine:
Instead of making the renaming the default in vcpkg how about putting it behind a feature named compatibility and have ports which require it depend on it? This allows the user to decide which naming schema they need and still offers the ports default naming. It could well be that downstream projects are already depending on the imp suffix. (Although CMake based projects do not care about it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's an exclusive choice (you can't have the renamed AND not renamed), we would prefer to always do things one way and make everything work correctly based on it.

With a different name for static and shared, downstream projects already could be depending on either of the names, so no matter what we do there will be some breakage (projects depending on static name won't build in shared, projects depending on shared name won't build in static). By always doing the rename, half the projects will change once, and then everyone will work in every configuration.

message(FATAL_ERROR "Curl's import library name should be consistent with the static library name.")
endif()

file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/lib/pkgconfig ${CURRENT_PACKAGES_DIR}/debug/lib/pkgconfig)
file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/debug/share)

if(VCPKG_LIBRARY_LINKAGE STREQUAL static)
if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/bin ${CURRENT_PACKAGES_DIR}/debug/bin)
else()
file(REMOVE ${CURRENT_PACKAGES_DIR}/bin/curl-config ${CURRENT_PACKAGES_DIR}/debug/bin/curl-config)
endif()

file(READ ${CURRENT_PACKAGES_DIR}/include/curl/curl.h CURL_H)
if(VCPKG_LIBRARY_LINKAGE STREQUAL static)
if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
string(REPLACE "#ifdef CURL_STATICLIB" "#if 1" CURL_H "${CURL_H}")
else()
string(REPLACE "#ifdef CURL_STATICLIB" "#if 0" CURL_H "${CURL_H}")
Expand Down
2 changes: 1 addition & 1 deletion ports/idevicerestore/CONTROL
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Source: idevicerestore
Version: 1.0.12-2
Version: 1.0.12-3
Description: Restore/upgrade firmware of iOS devices
Build-Depends: libimobiledevice, curl, libirecovery, libzip
40 changes: 0 additions & 40 deletions ports/idevicerestore/libcurl_imp.patch
View file
Open in desktop

This file was deleted.

13 changes: 1 addition & 12 deletions ports/idevicerestore/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,9 @@ vcpkg_from_github(
REF 1.0.12
SHA512 ba623be56c2f37853516d7d4c32e16f1ec72f33d512f18aa812ce6830af4b9e389f7af5321888dd0ddd168e282b652e379b60f90970680e213eabf489f406915
HEAD_REF msvc-master
PATCHES libcurl_d.patch
)

if(${VCPKG_LIBRARY_LINKAGE} MATCHES dynamic)
vcpkg_apply_patches(
SOURCE_PATH ${SOURCE_PATH}
PATCHES
libcurl_imp.patch)
else()
vcpkg_apply_patches(
SOURCE_PATH ${SOURCE_PATH}
PATCHES
libcurl_d.patch)
endif()

vcpkg_install_msbuild(
SOURCE_PATH ${SOURCE_PATH}
PROJECT_SUBPATH idevicerestore.vcxproj
Expand Down
2 changes: 1 addition & 1 deletion ports/libideviceactivation/CONTROL
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Source: libideviceactivation
Version: 1.2.68
Version: 1.2.68-1
Description: A library to handle the activation process of iOS devices
Build-Depends: libimobiledevice, libxml2, curl
40 changes: 0 additions & 40 deletions ports/libideviceactivation/libcurl_imp.patch
View file
Open in desktop

This file was deleted.

15 changes: 2 additions & 13 deletions ports/libideviceactivation/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,12 @@ vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY ONLY_DYNAMIC_CRT)
vcpkg_from_github(
OUT_SOURCE_PATH SOURCE_PATH
REPO libimobiledevice-win32/libideviceactivation
REF v1.2.68
REF v1.2.68
SHA512 c2742bba2d90c21e853255c9ef1b9a63560c3e65541a0a3daaace9b0c48d236b7947008dbcd6e42622251015b686758ebc6b564e379d831cb4f52af812430140
HEAD_REF msvc-master
PATCHES libcurl_d.patch
)

if(${VCPKG_LIBRARY_LINKAGE} MATCHES dynamic)
vcpkg_apply_patches(
SOURCE_PATH ${SOURCE_PATH}
PATCHES
libcurl_imp.patch)
else()
vcpkg_apply_patches(
SOURCE_PATH ${SOURCE_PATH}
PATCHES
libcurl_d.patch)
endif()

vcpkg_install_msbuild(
SOURCE_PATH ${SOURCE_PATH}
PROJECT_SUBPATH libideviceactivation.sln
Expand Down