[curl] Revert revert of -imp suffix removal.
#6698
Conversation
ras0219-msft
added
the
info:internal
| @@ -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" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I should have written "libcurl.lib or libcurl-d.lib based on configuration". My change doesn't remove the -d suffix :)
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
|
/azp run |
|
Pending fix in #6872 |
|
/azp run |
1 similar comment
|
/azp run |
* [curl] Revert revert of `-imp` suffix removal. * [idevicerestore][libideviceactivation] Simplify dependency on curl * [curl] Trivial change to provoke rebuild
No description provided.