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

Jump to bottom

[vcpkg_setup_pkgconfig_path] Add new functions to set or restore pkgconfig related environment variables #23429

Merged
merged 22 commits into from
May 10, 2022
Merged

[vcpkg_setup_pkgconfig_path] Add new functions to set or restore pkgconfig related environment variables #23429

merged 22 commits into from
May 10, 2022

Conversation

JackBoosY
Copy link
Contributor

ENV{PKG_CONFIG_PATH} shouldn't be added again.

Fixes #22812.

@JackBoosY JackBoosY added info:internal This PR or Issue was filed by the vcpkg team. category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly labels Mar 8, 2022
@JackBoosY
Copy link
Contributor Author

@jcc10 Can you please test this PR?

Thanks.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 8, 2022

I think we need to decide whether we want to prepend or to append. The current behaviour is to append, but normally vcpkg packages should be preferred over system packages AFAIU.

@Neumann-A
Copy link
Contributor

vcpkg/scripts/cmake/vcpkg_configure_make.cmake

Line 770 in 5962cf5

set(ENV{PKG_CONFIG_PATH} "${pkgconfig_installed_dir}${VCPKG_HOST_PATH_SEPARATOR}${pkgconfig_installed_share_dir}${VCPKG_HOST_PATH_SEPARATOR}$ENV{PKG_CONFIG_PATH}")

PREPEND it should be.

@Neumann-A
Copy link
Contributor

but really. The pkgconfig setup should be turned into its own function to make it consistent everywhere.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Mar 8, 2022 •
edited
Loading

but really. The pkgconfig setup should be turned into its own function to make it consistent everywhere.

Agreed.

EDIT: also, ENV{PKG_CONFIG} should be backup.

@JackBoosY JackBoosY closed this Mar 8, 2022
@JackBoosY JackBoosY reopened this Mar 8, 2022
@JackBoosY JackBoosY changed the title [vcpkg_configure_meson] Fix append host path [vcpkg_setup_pkgconfig_path] Add new functions to set or restore pkgconfig related environment variables Mar 8, 2022
dg0yt
dg0yt suggested changes Mar 9, 2022
View reviewed changes
scripts/cmake/vcpkg_configure_make.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor Author

JackBoosY commented Mar 10, 2022 •
edited
Loading

ncurses: mismatched debug and release lib count. Related: #23476. Fixing in #23478.

python2 / python3: Missing installation of bz2 dynamic library in debug mode. bz2.so with python2 and _bz2.cpython-310-x86_64-linux-gnu.so with python3. Can't repro.

libgnutls:

In procedure dynamic-link: file: "/mnt/vcpkg-ci/buildtrees/libgnutls/x64-linux-dbg/guile/src/guile-gnutls-v-2", message: "file not found"

Same issue in the upstream: https://www.mail-archive.com/[email protected]/msg01418.html
Fixing in #23478.

This was referenced Mar 10, 2022
Merged
Closed
@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Mar 11, 2022
@JackBoosY
Copy link
Contributor Author

Depends on #23478.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Apr 14, 2022
@JackBoosY JackBoosY linked an issue Apr 14, 2022 that may be closed by this pull request
[icu] build failure icu 70.1, error: Host path separator (:) in path; this is unsupported. #24138
Closed
@JackBoosY JackBoosY mentioned this pull request Apr 14, 2022
Closed
strega-nil
strega-nil approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

Bringing it up in the meeting

scripts/cmake/vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
@strega-nil-ms
Copy link
Contributor

This should be done with a private z_ name, so that we can change it in the future.

strega-nil-ms
strega-nil-ms previously requested changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

(make it internal)

@strega-nil-ms strega-nil-ms added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines requires:discussion labels Apr 14, 2022
@BillyONeal BillyONeal dismissed strega-nil-ms’s stale review April 28, 2022 00:44

strega-nil signed off.

BillyONeal
BillyONeal requested changes Apr 28, 2022
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I'm "request changes" over what look like extra/missing remaining calls trying to backup/restore PKG_CONFIG_PATH with vcpkg_restore_env_variables. (The documentation changes are nitpicks)

scripts/cmake/vcpkg_configure_qmake.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_configure_qmake.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_configure_meson.cmake Outdated Show resolved Hide resolved
scripts/cmake/z_vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
scripts/cmake/z_vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
scripts/cmake/z_vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
scripts/cmake/z_vcpkg_setup_pkgconfig_path.cmake Outdated Show resolved Hide resolved
@BillyONeal
Copy link
Member

The x86-windows failures don't look related to this so I'm going to azp run this again as soon as the other platforms finish building...

@BillyONeal
Copy link
Member

/azp run

1 similar comment
@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal BillyONeal merged commit 0d7603c into microsoft:master May 10, 2022
@BillyONeal
Copy link
Member

Thanks!

@JackBoosY JackBoosY deleted the dev/jack/22812 branch May 10, 2022 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Neumann-A Neumann-A Neumann-A left review comments

@strega-nil-ms strega-nil-ms strega-nil-ms left review comments

@dg0yt dg0yt dg0yt requested changes

@BillyONeal BillyONeal BillyONeal approved these changes

@strega-nil strega-nil strega-nil approved these changes

@github-actions github-actions[bot] github-actions[bot] approved these changes

@LilyWangLL LilyWangLL LilyWangLL approved these changes

Assignees

@LilyWangLL LilyWangLL

Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Milestone
No milestone
9 participants
@JackBoosY @dg0yt @Neumann-A @strega-nil-ms @BillyONeal @strega-nil @LilyWangLL @dan-shaw @vicroms