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.

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

[scripts] add new function vcpkg_fixup_pkgconfig #9861

Merged
merged 12 commits into from
Apr 28, 2020

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Jan 31, 2020

similar to vcpkg_fixup_cmake_targets.
Mainly prefix corrections and transforming absolute paths into relativ ones (relative to prefix).

tries to solve #3108

use --define-variable=prefix=INSTALL_PATH
@vicroms vicroms added info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels Feb 5, 2020
@vicroms
Copy link
Member

vicroms commented Feb 5, 2020

/azp run

@Neumann-A
Copy link
Contributor Author

@vicroms
set(ENV{PKG_CONFIG} "${PKGCONFIG} --define-variable=prefix=${_VCPKG_INSTALLED}")
seems to be always required since
./pkg-config.exe --exists --print-errors "xproto"
will always print
Variable 'prefix' not defined in 'd:/ma ke/installed/x64-windows/lib/pkgconfig\xproto.pc' if there is no prefix variable in it. Otherwise the reseating of the prefix is done automatically on windows even when prefix=nonsens with pkgconfig v0.29.2
pkg-config.exe --variable=prefix xproto -> d:/ma\ ke/installed/x64-windows
while on linux the result is (v0.28.0)
pkg-config.exe --variable=prefix xproto -> nonsens

@Neumann-A
Copy link
Contributor Author

So my question now would be:
Should we let the prefix be defined on windows to leverage on the automatic prefix correction?
(while other OSes are required to use --define-variable=prefix=${_VCPKG_INSTALLED})

@vicroms
Copy link
Member

vicroms commented Feb 5, 2020

We want to leverage the automatic correction, so if there's no other option we should define it as this PR did originally.

I think that internally we should pass --define-variable=prefix=${_VCPKG_INSTALLED} in our helper functions regardless of platform. For external consumers we could do something similar to the usage message we have for CMake config files and tell them to define the prefix.

@ras0219-msft
Copy link
Contributor

set(ENV{PKG_CONFIG} "${PKGCONFIG} --define-variable=prefix=${_VCPKG_INSTALLED}")

+1 for this.

Just to reiterate the underlying sentiment and goal: we don't want packages to magically work on the original build machine, but randomly fail if you try to use them on another machine. Instead, we want them to fail consistently in both environments or work in both. If that requires users to throw --define-variable=prefix=..., so be it; we will find ways of making that more ergonomic.

# .. command:: vcpkg_fixup_pkgconfig
#
# Tries to fix the paths found in *.pc files

Copy link
Contributor

Choose a reason for hiding this comment

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

As a note: we use a different comment style, which is extracted into .md files via docs/regenerate.ps1. See some other helpers like vcpkg_configure_cmake() for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed but still not sure if its 100% correct.

@Neumann-A
Copy link
Contributor Author

I'll add in a check which will check if the following:

Libs: -L${libdir} -lfreetype
Libs.private: 

really exist. Translates -l into lib.(a|lib) or .(a|lib) must exist. Most commonly, the debug name is false for CMake builds due to added debug suffixes while the library name is hardcoded in a *.pc.in

The same for

Requires:
Requires.private: zlib, bzip2, libpng

which means that a .pc file needs to exist

@MVoz
Copy link
Contributor

MVoz commented Mar 17, 2020

@dan-shaw @ras0219-msft
merge!
in the course of work, any shortcomings will be identified, need to merge
the script does not yet carry anything without accessing it

Copy link
Contributor

@MVoz MVoz left a comment

Choose a reason for hiding this comment

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

-- Fixing pkgconfig --- finished
CMake Error at scripts/cmake/vcpkg_fixup_pkgconfig.cmake:245 (set):
set given invalid arguments for CACHE mode.

Note: since CMake is run in script mode the description of VCPKG_FIXUP_PKGCONFIG_CALLED cannot be viewed
@MVoz
Copy link
Contributor

MVoz commented Mar 23, 2020

when there are dependencies, the script does not work, and outputs an error without pointing to the package.
I performed validation of all files

removed under comment
# vcpkg_fixup_pkgconfig_check_libraries("DEBUG" _contents "${_vfpkg_SYSTEM_LIBRARIES}" "${_vfpkg_SYSTEM_PACKAGES}")
# vcpkg_fixup_pkgconfig_check_libraries("RELEASE" _contents "${_vfpkg_SYSTEM_LIBRARIES}" "${_vfpkg_SYSTEM_PACKAGES}")

@Neumann-A
Copy link
Contributor Author

Could you add the *.pc file which failed?
I am not going to comment vcpkg_fixup_pkgconfig_check_libraries out since its intention is too make sure that all required packages and libraries are available.

@MVoz
Copy link
Contributor

MVoz commented Mar 25, 2020

I am not suggesting that you comment on this line \ function
I removed it under the comment (at itself), so that the build was successful until I find out the reason for the error

collected the glib(MESON) package,*.pc everything from vcpkg, all pass validation manually
I don't know what the reason is, there is no debugging information

@Neumann-A
Copy link
Contributor Author

until I find out the reason for the error
an error without pointing to the package.

So the reason for the error was that one package was missing but the error did not point to the missing package? Or am I misunderstanding it again?

@MVoz
Copy link
Contributor

MVoz commented Mar 25, 2020

all packages were found and the build was made successfully, the script after the build can not work out some system files
which ones, I have no idea, I got the error without explanation

пакеты все найдены и сборка была произведена успешно, скрипт после сборки не может отработать некие системные файлы
какие именно, я без понятия, вывела ошибку без пояснения

specifically these lines

# vcpkg_fixup_pkgconfig_check_libraries("DEBUG" _contents "${_vfpkg_SYSTEM_LIBRARIES}" "${_vfpkg_SYSTEM_PACKAGES}")
# vcpkg_fixup_pkgconfig_check_libraries("RELEASE" _contents "${_vfpkg_SYSTEM_LIBRARIES}" "${_vfpkg_SYSTEM_PACKAGES}")

@JackBoosY
Copy link
Contributor

/azp run

@Neumann-A
Copy link
Contributor Author

Neumann-A commented Apr 8, 2020

TODO:
#10731 (comment)
Make prefix use ${pcfiledir}/../..

Done

@Neumann-A Neumann-A marked this pull request as draft April 9, 2020 18:25
make pc files relocatable by using ${pcfiledir} in prefix
tested with x window pr
@Neumann-A Neumann-A marked this pull request as ready for review April 9, 2020 22:10
@JackBoosY
Copy link
Contributor

LGTM.
@ras0219-msft Could you review this PR?

Thanks.

@MVoz
Copy link
Contributor

MVoz commented Apr 10, 2020

@Neumann-A
still would such a feature to implement, naturally in another request

example

# Templates release\debug.pc.cmake \\ RELEASE and DEBUG

prefix=@CMAKE_INSTALL_PREFIX@ ### @CURRENT_INSTALLED_DIR@ ???
exec_prefix=${prefix}
libdir=${exec_prefix}/@LIB_INSTALL_DIR@
includedir=${prefix}/include ### ${exec_prefix}/@INCLUDE_INSTALL_DIR@ ???

Name: @PORT@
URL: @PORT_HOMEPAGE@
Description: @PORT_DESCRIPTION@ ### error /n
Version: @PORT_VERSION@ ### ???
Requires: @PORT_REQUIRES_RELEASE@

Libs: -L${libdir} @PORT_LIBS_RELEASE@ ### -lexample
Cflags: -I${includedir}


# Generate and Install Pkgconfig

configure_file(${scripts}/templates/release.pc.cmake ${PROJECT_BINARY_DIR}//@PORT@.pc @ONLY) ### ${PROJECT_BINARY_DIR} ???
install(FILES ${PROJECT_BINARY_DIR}/@PORT@.pc DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig") ### ${CMAKE_INSTALL_LIBDIR} ???

@MVoz
Copy link
Contributor

MVoz commented Apr 17, 2020

@ras0219-msft merge this PR

Thanks.

@strega-nil
Copy link
Contributor

LGTM! Thanks @Neumann-A :)

@strega-nil strega-nil merged commit c444db5 into microsoft:master Apr 28, 2020
@Neumann-A Neumann-A deleted the add_vcpkg_fixup_pkgconfig branch November 18, 2020 20:25
@traversaro traversaro mentioned this pull request Sep 25, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants