[scripts-audit] vcpkg_fixup_pkgconfig #19658

strega-nil · 2021-08-19T21:26:58Z

Redo of #19348 (since that was buggy)

ras0219-msft

I recommend testing this by:

Install a bunch of packages containing .pc files before this change
Install the same packages with this change into a different install dir
Diff all .pc files across the two. They must be the same.

At a minimum, I recommend 20 different packages with .pc files. I see a bunch of hits via git grep '\.pc' ports to use for this.

ras0219-msft · 2021-08-19T22:44:37Z

scripts/cmake/vcpkg_fixup_pkgconfig.cmake

    endif()
+    vcpkg_list(JOIN pkg_config_path "${VCPKG_HOST_PATH_SEPARATOR}" pkg_config_path)


Is this guaranteed to be a no-op when VCPKG_HOST_PATH_SEPARATOR is ;? What about in the face of empty elements?

@ras0219-msft it's actually not a no-op when VCPKG_HOST_PATH_SEPARATOR is ;: it removes one \ in front of ;s. In the face of empty elements, it is correct.

I think this ends up working by accident?

scripts/cmake/vcpkg_fixup_pkgconfig.cmake

dg0yt · 2021-08-20T21:54:02Z

Install a bunch of packages containing .pc files before this change

...
At a minimum, I recommend 20 different packages with .pc files. I see a bunch of hits via git grep '\.pc' ports to use for this.

Install gdal into a fresh root... it pulls in a lot of packages with pc files, because its configuration depends on it. And find some more ports which are not used by gdal.

Neumann-A · 2021-08-20T22:03:26Z

If the gtk chain fails it is probably also due to not working pc files. (mostly meson build system but some deps have cmake fallbacks)

dg0yt · 2021-08-20T22:08:14Z

scripts/cmake/vcpkg_fixup_pkgconfig.cmake

+        "${CURRENT_INSTALLED_DIR}${PATH_SUFFIX_${config}}/lib/pkgconfig"
+        "${CURRENT_INSTALLED_DIR}/share/pkgconfig"
+        "${CURRENT_PACKAGES_DIR}${PATH_SUFFIX_${config}}/lib/pkgconfig"
+        "${CURRENT_PACKAGES_DIR}/share/pkgconfig"


Only loosely related to this PR:
Is there a point in installing pc files to share/pkgconfig? Are there ports which do that? IIUC this location is unable to separate debug builds from release builds. This could be a subtle source for errors when pc files specify transitive dependencies as Libs, not as Requires.

I think xorg-proto does that. Basically it allows for arch independent data to be looked up. pc files can be and are use for more than just for finding libraries.

Okay...
So proper usage is to be ensured by review. Or is this another case for an automated check:
No Libs(.private) field in share/pkgconfig?

I think xorg-proto does that

There is no such port?

There is no such port?

#9966 the mother of all vcpkg pkgconfig stuff. Didn't have time to work on it and won't take the time unless it is clear how to integrate the x stuff into vcpkg without messing linux totally up ;)

strega-nil · 2021-08-23T23:48:15Z

Alright, with the new changes, the following ports have no changes besides ordering of lines (I just installed a bunch of ports that called vcpkg_fixup_pkgconfig) (on arm64-osx):

jack2
boost-io
freexl
mailio
tool-meson
cairomm
glib
blas
freealut
mbedtls[pthreads]
boost-integer
boost-iterator
boost-regex
graphene
lapack-reference[blas-select,noblas]
boost-math
tiff[jpeg,lzma,zip]
openblas
atk
libjpeg-turbo
json-c
sdl2
boost-rational
brotli
opengl
boost-core
libtheora
boost-array
openh264
boost-preprocessor
boost-concept-check
boost-vcpkg-helpers
szip
boost-tokenizer
cairo[fontconfig,freetype,gobject]
getopt
gmp
libffi
c-ares
zstd
gdk-pixbuf
fribidi
boost-lexical-cast
boost-smart-ptr
boost-build
trantor
boost-type-traits
boost-atomic
boost-assert
libtasn1
abseil
boost-container
boost-mpl
libsigcpp
libssh[crypto,mbedtls]
pcre
boost-asio
libvorbis
boost-static-assert
libjuice
boost-algorithm
pixman
libepoxy
console-bridge
boost-unordered
lzo
catch2
plog
opusfile
dirent
glew
vcpkg-cmake-config
pthreads
boost-config
freetype[bzip2]
pugixml
openssl
boost-ratio
qhull
fontconfig
boost-throw-exception
libiconv
boost-conversion
boost-move
boost-typeof
expat
vcpkg-cmake
boost-date-time
libsndfile[external-libs]
boost-modular-build-helper
boost-system
icu
boost-fusion
boost-lambda
boost-range
libdatachannel[ws]
rapidjson
libzip[bzip2]
boost-functional
boost-uninstall
zlib
boost-optional
boost-bind
protobuf
boost-container-hash
jsoncpp
speexdsp
boost-chrono
openal-soft
libpng
libgpg-error
boost-exception
boost-tuple
libzip[commoncrypto,default-aes]
boost-winapi
allegro5
physfs
minizip-ng
opus
boost-function-types
boost-detail
liblzma
bzip2
boost-function
boost-utility
libuuid
drogon
gettext
boost-intrusive
cfitsio
libogg
fann
boost-predef
freetype[brotli,png,zlib]
pthread
boost-align
boost-numeric-conversion
nlohmann-json
boost-type-index
pcre2
usrsctp
libflac

strega-nil-ms · 2021-08-24T16:47:17Z

/azp run

azure-pipelines · 2021-08-24T16:47:26Z

Azure Pipelines successfully started running 1 pipeline(s).

strega-nil-ms · 2021-08-24T18:21:29Z

Additionally, the same is true for x64-windows-static with gdal's dependencies (tho not gdal itself, since gdal:x64-windows-static fails to build).

dg0yt · 2021-08-24T21:00:27Z

gdal:x64-windows-static fails to build

Where does it fail now?

dg0yt · 2021-08-24T21:17:24Z

gdal:x64-windows-static fails to build

Where does it fail now?

Anyway, gdal:x64-windows-static doesn't use pc files. That's a different story. A lot of open source GIS-related ports use msbuild for windows (non-mingw) and lack a mechanism to exchange usage requirements between ports.
But gdal:x64-mingw-static uses pc files.

strega-nil-ms · 2021-09-08T00:09:31Z

@dg0yt yeah I just wanted to test with x64-windows-static that it worked, gdal isn't necessarily that important.

BillyONeal · 2021-09-08T19:50:38Z

scripts/cmake/vcpkg_fixup_pkgconfig.cmake

+    if("${VCPKG_HOST_PATH_SEPARATOR}" STREQUAL ";")
+        vcpkg_list(APPEND pkg_config_path $ENV{PKG_CONFIG_PATH})
    else()
-        set(ENV{PKG_CONFIG_PATH} "${PKGCONFIG_INSTALLED_DIR}${VCPKG_HOST_PATH_SEPARATOR}${PKGCONFIG_INSTALLED_SHARE_DIR}${VCPKG_HOST_PATH_SEPARATOR}${PKGCONFIG_PACKAGES_DIR}${VCPKG_HOST_PATH_SEPARATOR}${PKGCONFIG_PACKAGES_SHARE_DIR}")
+        vcpkg_list(JOIN pkg_config_path "${VCPKG_HOST_PATH_SEPARATOR}" pkg_config_path)
+        if(NOT "$ENV{PKG_CONFIG_PATH}" STREQUAL "")
+            string(APPEND pkg_config_path "${VCPKG_HOST_PATH_SEPARATOR}$ENV{PKG_CONFIG_PATH}")
+        endif()
    endif()


It seems like the imbuing of pkg_config_path with $ENV{PKG_CONFIG_PATH} should be done as a separate step from transforming ; to ${VCPKG_HOST_PATH_SEPARATOR} ?

How about (with no ifs):

vcpkg_list(APPEND pkg_config_path $ENV{PKG_CONFIG_PATH}) vcpkg_list(JOIN pkg_config_path "${VCPKG_HOST_PATH_SEPARATOR}" pkg_config_path)

?

The reason we don't want this is:

Let PKG_CONFIG_PATH = [a;b, c, d]

Assume VCPKG_HOST_PATH_SEPARATOR = ';'. Then, PKG_CONFIG_PATH = 'a\;b;c;d', and you get:

vcpkg_list(APPEND pkg_config_path $ENV{PKG_CONFIG_PATH}) # pkg_config_path = "...;a\;b;c;d" vcpkg_list(JOIN pkg_config_path "${VCPKG_HOST_PATH_SEPARATOR}" pkg_config_path) # pkg_config_path = "...;a;b;c;d"

thus, pkg_config_path is wrong.

I think your "Let PKG_CONFIG_PATH" part already starts with pkg_config_path being wrong, since "a, b, c" isn't a valid PATH entry.

that was my attempt at array list syntax; basically, let PKG_CONFIG_PATH be the elements:

a;b

c

d

I wouldn't mind if ;wasn't supported in directory names: It is the separator on Unix anyway. But : and do matter.
Again, I would suggest a test-first approach. Someone will come and break it. Guard it with a unit test.

BillyONeal

This seems OK to me but I am far Far FAR from a pkgconfig expert.

github-actions

You have modified or added at least one portfile where deprecated functions are used.

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

scripts/test_ports/unit-test-cmake/portfile.cmake

dg0yt · 2021-09-16T20:02:22Z

Can you add a check that portfiles don't contain items matching

^(-l)?(optimized|debug)$
::

This is typical for pc files from CMake config, and possibly only found much later. Example: #20062 (comment)

(We might eventually resolve the optimized/debug variants.)

strega-nil-ms · 2021-09-23T21:36:35Z

@dg0yt given that that's not really a part of the scripts audit (and that it is in theory fine), I'd prefer not to.

dg0yt · 2021-09-23T22:00:45Z

@strega-nil-ms Okay. Finishing the audit in a short time frame is also fine. (I consider the scripts locked for other changes as long as there is an audit going on.)

scripts/cmake/vcpkg_host_path_list.cmake

scripts/cmake/vcpkg_fixup_pkgconfig.cmake

github-actions bot approved these changes Aug 19, 2021

View reviewed changes

JonLiu1993 assigned PhoebeHui Aug 20, 2021

PhoebeHui added category:scripts-audit Part of the scripts audit effort. info:internal This PR or Issue was filed by the vcpkg team. labels Aug 20, 2021

ras0219-msft reviewed Aug 20, 2021

View reviewed changes

dg0yt reviewed Aug 20, 2021

View reviewed changes

strega-nil-ms force-pushed the vcpkg_fixup_pkgconfig branch from 0a2d444 to cca3b43 Compare August 23, 2021 23:48

github-actions bot approved these changes Aug 23, 2021

View reviewed changes

github-actions bot approved these changes Aug 24, 2021

View reviewed changes

strega-nil-ms mentioned this pull request Sep 8, 2021

[vcpkg_fixup_pkgconfig] Add support for Cflags.private #19852

Merged

BillyONeal reviewed Sep 8, 2021

View reviewed changes

BillyONeal approved these changes Sep 8, 2021

View reviewed changes

github-actions bot approved these changes Sep 15, 2021

View reviewed changes

strega-nil-ms force-pushed the vcpkg_fixup_pkgconfig branch 2 times, most recently from 6b44fa0 to a2777c6 Compare September 15, 2021 22:14

github-actions bot approved these changes Sep 15, 2021

View reviewed changes

github-actions bot approved these changes Sep 16, 2021

View reviewed changes

strega-nil-ms force-pushed the vcpkg_fixup_pkgconfig branch from 560d63c to 3f7482e Compare September 23, 2021 20:45

github-actions bot approved these changes Sep 23, 2021

View reviewed changes

strega-nil-ms mentioned this pull request Sep 23, 2021

[scripts-audit] Make buildsystem #20165

Merged

1 task

ras0219-msft approved these changes Sep 23, 2021

View reviewed changes

scripts/cmake/vcpkg_host_path_list.cmake Show resolved Hide resolved

scripts/cmake/vcpkg_fixup_pkgconfig.cmake Show resolved Hide resolved

github-actions bot approved these changes Sep 23, 2021

View reviewed changes

strega-nil-ms force-pushed the vcpkg_fixup_pkgconfig branch from 4a889d9 to 7d84df0 Compare September 23, 2021 23:07

github-actions bot approved these changes Sep 23, 2021

View reviewed changes

strega-nil added 10 commits September 24, 2021 12:19

[scripts-audit] vcpkg_fixup_pkgconfig

1c8c656

Neumann-A CR, fix docs

13eb344

vcpkg_fixup_pkgconfig bugs

387bb16

fix group

0119994

moar fixing

4c9e10e

be more clever around pkg_config_path

e999ee6

add vcpkg_host_path_list so that we can unit test

ea57687

move stuff around a bit

04f1b69

fix bug in vcpkg_host_path_list.cmake

745c379

ras0219 CRs

e6ac1c0

strega-nil-ms force-pushed the vcpkg_fixup_pkgconfig branch from 7d84df0 to e6ac1c0 Compare September 24, 2021 19:25

github-actions bot approved these changes Sep 24, 2021

View reviewed changes

strega-nil-ms merged commit f0281bf into microsoft:master Sep 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scripts-audit] vcpkg_fixup_pkgconfig #19658

[scripts-audit] vcpkg_fixup_pkgconfig #19658

strega-nil commented Aug 19, 2021

ras0219-msft left a comment

ras0219-msft Aug 19, 2021

strega-nil Aug 23, 2021 •

edited

Loading

dg0yt commented Aug 20, 2021

Neumann-A commented Aug 20, 2021

dg0yt Aug 20, 2021

Neumann-A Aug 20, 2021

dg0yt Aug 20, 2021

autoantwort Aug 23, 2021

Neumann-A Aug 23, 2021

strega-nil commented Aug 23, 2021 •

edited by strega-nil-ms

Loading

strega-nil-ms commented Aug 24, 2021

azure-pipelines bot commented Aug 24, 2021

strega-nil-ms commented Aug 24, 2021 •

edited

Loading

dg0yt commented Aug 24, 2021

dg0yt commented Aug 24, 2021

strega-nil-ms commented Sep 8, 2021 •

edited

Loading

BillyONeal Sep 8, 2021

strega-nil-ms Sep 8, 2021

BillyONeal Sep 8, 2021

strega-nil-ms Sep 10, 2021

dg0yt Sep 11, 2021

BillyONeal left a comment

github-actions bot left a comment

dg0yt commented Sep 16, 2021

strega-nil-ms commented Sep 23, 2021

dg0yt commented Sep 23, 2021

		endif()
		vcpkg_list(JOIN pkg_config_path "${VCPKG_HOST_PATH_SEPARATOR}" pkg_config_path)

[scripts-audit] vcpkg_fixup_pkgconfig #19658

[scripts-audit] vcpkg_fixup_pkgconfig #19658

Conversation

strega-nil commented Aug 19, 2021

ras0219-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strega-nil Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

dg0yt commented Aug 20, 2021

Neumann-A commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strega-nil commented Aug 23, 2021 • edited by strega-nil-ms Loading

strega-nil-ms commented Aug 24, 2021

azure-pipelines bot commented Aug 24, 2021

strega-nil-ms commented Aug 24, 2021 • edited Loading

dg0yt commented Aug 24, 2021

dg0yt commented Aug 24, 2021

strega-nil-ms commented Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

dg0yt commented Sep 16, 2021

strega-nil-ms commented Sep 23, 2021

dg0yt commented Sep 23, 2021

strega-nil Aug 23, 2021 •

edited

Loading

strega-nil commented Aug 23, 2021 •

edited by strega-nil-ms

Loading

strega-nil-ms commented Aug 24, 2021 •

edited

Loading

strega-nil-ms commented Sep 8, 2021 •

edited

Loading