[vcpkg_fixup_pkgconfig] Handle spaces in path, do not validate individual libraries #13126
Conversation
Add test for zlib with spaces in path.
|
The path used in #13105 is the appveyor standard path of |
|
cc @Neumann-A for review this PR. |
JackBoosY
added
category:tool-update
|
Sadly the build continues to fail. |
|
|
||
| set(BACKUP_ENV_PKG_CONFIG_PATH "$ENV{PKG_CONFIG_PATH}") | ||
| if(ENV{PKG_CONFIG_PATH}) |
There was a problem hiding this comment.
| if(ENV{PKG_CONFIG_PATH}) | |
| if(DEFINED ENV{PKG_CONFIG_PATH}) |
checking for ENV variables requires DEFINED
| endif() | ||
| endforeach() | ||
|
|
||
| set(CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES_BACKUP}) | ||
| set(ENV{PKG_CONFIG_PATH} "${BACKUP_ENV_PKG_CONFIG_PATH}") |
There was a problem hiding this comment.
I always wonder if this is ok and undefines ENV{PKG_CONFIG_PATH} if "${BACKUP_ENV_PKG_CONFIG_PATH}" is empty or if it defines it to an empty string
There was a problem hiding this comment.
This will define it to empty string, but I don't think undefined vs empty string is something that can be reasonably distinguished; enough programs consider them equivalent that it's unreasonable to require the user to use one or the other.
There was a problem hiding this comment.
undefined vs empty string is something that can be reasonably distinguished
it can and is. Empty string in path like variables typically mean ./ so the current working directory. I have seen issue with it in vcpkg_configure_make once which is why this whole ENV checking and setting/appending is required.
nope since the error is not in the check but in the replacement before. Prefix was somehow not correctly replaced.
that is potentially dangerous since LFLAGS might contain gcc specific linker flags. (-Wl,something,someotherthing) It won't affect build with autotools but meson and cmake builds will most likely choke on it on Windows. |
| debug_message("${_libname} detected as an existing full path!") | ||
| continue() # fullpath in -l argument and exists; all ok | ||
| set(SEARCH_PATHS) | ||
| string(REGEX MATCHALL "([^ \t\\\\]+|\\\\.)+" LIBS_ARGS "${_pkg_libs_output}") |
There was a problem hiding this comment.
([^ \t\\\\]+|\\\\.)+
will probably fail with:
"mypath\ -Lotherpath\ -llib"
There was a problem hiding this comment.
Ah you assume that the path separator is / ?
There was a problem hiding this comment.
The desired outcome of that regex is to parse all that as a single blob -- which is how it is intended to be parsed afaik:
mypath\ -Lotherpath\ -llib->mypath\ -Lotherpath\ -llibmypath\ -Lotherpath -llib->mypath\ -Lotherpath;-llibmypath -Lotherpath -llib->mypath;-Lotherpath;-llib
There was a problem hiding this comment.
Yeah, in my local environment, pkg-config is emitting / for all path separators.
| debug_message("pkg-config output:${_pkg_lib_paths_output}") | ||
| debug_message("pkg-config error output:${_pkg_error_out}") | ||
| if(VCPKG_TARGET_IS_WINDOWS) | ||
| list(APPEND CMAKE_FIND_LIBRARY_SUFFIXES .lib .dll.a .a) |
There was a problem hiding this comment.
still wondering if we should allow .dll.lib as a suffix since this is the default for autotools shared builds which get currently renamed. The only issue with that is that it must also be set in vcpkg.cmake
There was a problem hiding this comment.
My not-fully-informed position is that we should always rename them all to be just .lib / .dll when targeting Windows and not Mingw. However, I don't have all the facts to be completely sure of that.
There was a problem hiding this comment.
The naming probably originates from wanting to have static/shared libraries in the same directory. There is some story about it in meson including a discussion of the names of the generated pdbs which is why they name static libraries as lib<name>.a by default on windows.
| if(LIBS_ARG MATCHES "^-L(.*)") | ||
| list(APPEND SEARCH_PATHS "${CMAKE_MATCH_1}") |
There was a problem hiding this comment.
The reason I ask pkg-config for the search paths is to make sure pkg-config returns the correct string.
The problem currently is that if libdir contains spaces due to the prefix being on a path with spaces pkg-config is not returning the correct path.
One solution is to transform -L${libdir} to -L"${libdir}" in the steps before. the checks.
There was a problem hiding this comment.
From what I'm seeing, pkg-config emits -LC:/src/vcpkg\ space/installed/x86-windows/lib when presented with spaces (C:\src\vcpkg space as the vcpkg root).
There was a problem hiding this comment.
Ok that is different from what the msys pkg-config. The msys version was just returning -LC:/src/vcpkg in these cases. Can cmake correctly deal with the escaped spaces \ ? Otherwise you need to replace that with a non escaped version
There was a problem hiding this comment.
Because CMake uses semicolons to delimit lists, I think it "correctly deals with escaped spaces" by sidestepping the issue entirely. Instead, it means the resulting items get the escaped forms verbatim, which is why this change manually evaluates them.
My current thinking is that this is just acceptable tradeoffs, since we can't really intend to keep up with every compiler's command line arguments and pkg-config allows users to reasonably include compiler-specific flags. |
root@5178487134bb:/vcpkg# cat "/packaG es/zlib_x64-linux/lib/pkgconfig/zlib.pc"
prefix=${pcfiledir}/../..
exec_prefix=${prefix}
libdir=${prefix}/lib
sharedlibdir=${prefix}/lib
includedir=${prefix}/include
Name: zlib
Description: zlib compression library
Version: 1.2.11
Requires:
Libs: -L${libdir} -L${sharedlibdir} -lz
Cflags: -I${includedir}
root@5178487134bb:/vcpkg# PKG_CONFIG_PATH=/packaG\ es/zlib_x64-linux/lib/pkgconfig pkg-config --libs zlib
-L/packaG es/zlib_x64-linux/lib/pkgconfig/../../lib -lz
root@5178487134bb:/vcpkg# PKG_CONFIG_PATH=/packaG\ es/zlib_x64-linux/lib/pkgconfig pkg-config --libs-only-L zlib
-L/packaG
root@5178487134bb:/vcpkg# PKG_CONFIG_PATH=/packaG\ es/zlib_x64-linux/lib/pkgconfig pkg-config --libs-only-other zlib
es/zlib_x64-linux/lib/pkgconfig/../../lib:( it looks like |
|
@ras0219 Regex replace |
|
This fixes #13166. |
Yeah, this appears to work: string(REGEX REPLACE " -L(\\\${[^}]*}[^ \n\t]*)" " -L\"\\1\"" _contents "${_contents}")
string(REGEX REPLACE " -I(\\\${[^}]*}[^ \n\t]*)" " -I\"\\1\"" _contents "${_contents}")
string(REGEX REPLACE " -l(\\\${[^}]*}[^ \n\t]*)" " -l\"\\1\"" _contents "${_contents}") |
|
Ok, so on Windows I believe there's a double-escaping bug that does the right thing if there aren't quotes, but if you actually quote things it causes pkg-config to spit out stuff like I can hack and hardcode things to unescape triple-slash correctly, but the real question is who's going to consume this? Meson? Autoconf in msys2? How do they handle this? |
…roschuma/pkgconfig-spaces
…roschuma/pkgconfig-spaces
…roschuma/pkgconfig-spaces
…roschuma/pkgconfig-spaces
…roschuma/pkgconfig-spaces
…roschuma/pkgconfig-spaces
…roschuma/pkgconfig-spaces
|
I just asked @JackBoosY if his concerns are still outstanding |
|
LGTM, @Neumann-A do you have other questions about this changes? |
|
@JackBoosY No |
|
Thanks for your help folks! |
|
Thanks again for your contribution Robert! |
|
TYVM for pushing it through :) |
Because pkgconfig's recommended approach when using libraries outside pkgconfig is to add them to your
.pcfile, it is impossible to enumerate the set of external libraries on all possible systems. Therefore we cannot realistically expect libraries to resolve to those inside the current package or installed dir. We runpkgconfig --existsto ensure that the .pc file is able to be loaded in the current environment, but perform no validation of the underlying libraries.In order for the output of
pkg-configto correctly handle spaces in the path, all path-based flags in the.pcfile must be quoted:As a drive by, this PR also adds an end-to-end test for zlib with spaces in path.
Fixes bzip2 build failure #13103
Fixes [brotli] build failure #13166
Supercedes [sdl2] Fix pkgconfig: missing system libs #13551
This may also fix other issues as well due to significant logical simplification.
@Neumann-A for review to ensure this still covers all the important cases -- For example, this currently leaves off handling of
-pthreadand instead ignores unrecognized arguments