Skip to content

[scripts-audit] vcpkg_find_acquire_program#21173

Merged
BillyONeal merged 19 commits intomicrosoft:masterfrom
strega-nil:vcpkg_find_acquire_program
Nov 11, 2021
Merged

[scripts-audit] vcpkg_find_acquire_program#21173
BillyONeal merged 19 commits intomicrosoft:masterfrom
strega-nil:vcpkg_find_acquire_program

Conversation

@strega-nil-ms
Copy link
Contributor

@strega-nil-ms strega-nil-ms commented Nov 3, 2021

This is a PR for the actual changes to vcpkg_find_acquire_program, rather than trying to completely change how it works

@strega-nil-ms strega-nil-ms force-pushed the vcpkg_find_acquire_program branch from 9fa1ce7 to 4f2c899 Compare November 3, 2021 20:03
@strega-nil
oops, extra ""
6ed3cb9 
make some changes to names too
@strega-nil-ms strega-nil-ms mentioned this pull request Nov 4, 2021
Closed
51 tasks
@BillyONeal
Copy link
Member

BillyONeal commented Nov 8, 2021

qca regressions were fixed by #21250

@BillyONeal
Copy link
Member

BillyONeal commented Nov 10, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Nov 10, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal BillyONeal merged commit 86b2385 into microsoft:master Nov 11, 2021
@dg0yt dg0yt mentioned this pull request Nov 11, 2021
Merged
dg0yt
dg0yt reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

I guess this broke building fontconfig.
#21026 (comment)

scripts/cmake/vcpkg_find_acquire_program.cmake
if(NOT ${VAR})
find_program(${VAR} ${PROGNAME})
if(${VAR} AND NOT ${VAR}_VERSION_CHECKED)
do_version_check()
Copy link
Contributor

@dg0yt dg0yt Nov 11, 2021

Choose a reason for hiding this comment

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

Here, there was a version check when the program was found from ENV{PATH}...

scripts/cmake/vcpkg_find_acquire_program.cmake
z_vcpkg_find_acquire_program_find_internal("${program}"
INTERPRETER "${interpreter}"
PATHS ${paths_to_search}
NAMES ${search_names}
Copy link
Contributor

@dg0yt dg0yt Nov 11, 2021

Choose a reason for hiding this comment

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

... but here, programs from apt or brew are no longer checked for version.

@BillyONeal
Copy link
Member

BillyONeal commented Nov 11, 2021 •
edited
Loading

Quoth @cenit : #21026 (comment)

after this PR fontconfig has been broken on osx/linux in all PR

#20658 #20185

RE: @dg0yt:

I guess this broke building fontconfig.

Any ideas why we got a passing run here then?

@cenit
Copy link
Contributor

cenit commented Nov 11, 2021

my bad, my bisect was wrong

@Neumann-A
Copy link
Contributor

Neumann-A commented Nov 11, 2021

Any ideas why we got a passing run here then?

missing dep on tool-meson and the tool was probably downloaded and thus cached

Starting package 171/886: tool-meson:x64-linux
Building package tool-meson[core]:x64-linux...
-- Found meson('0.53.2') but at least version 0.58.1 is required! Trying to use internal version if possible!
-- Downloading https://github.com/mesonbuild/meson/archive/aeda7f249c4a5dbbecc52e44f382246a2377b5b0.tar.gz -> meson-aeda7f249c4a5dbbecc52e44f382246a2377b5b0.tar.gz...
-- Using meson: /usr/bin/python3;/usr/bin/meson
-- Performing post-build validation
-- Performing post-build validation done

Using meson: /usr/bin/python3;/usr/bin/meson is strange if the internal stuff gets printed

@dg0yt
Copy link
Contributor

dg0yt commented Nov 11, 2021

Any ideas why we got a passing run here the

Not exactly. But at least before this PR, vfap was not idempotent. (IIRC you never got the system version if a downloaded version was present.)

I tested fontconfig:x64-linux on a older state, before and after removing meson from downloads/tools. It does detect that a download is needed.

-- Found meson('') but at least version 0.58.1 is required! Trying to use internal version if possible!
-- Using cached meson-aeda7f249c4a5dbbecc52e44f382246a2377b5b0.tar.gz.

Will repeat with master now.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 11, 2021

Well, I even didn't have meson installed until now. Now the results are even more interesting...

On first run:

-- Found meson('0.45.1') but at least version 0.58.1 is required! Trying to use internal version if possible!
-- Using cached meson-aeda7f249c4a5dbbecc52e44f382246a2377b5b0.tar.gz.
-- Configuring x64-linux-dbg
CMake Error at scripts/cmake/vcpkg_execute_required_process.cmake:127 (message):
    Command failed: /usr/bin/python3 /usr/bin/meson --buildtype plain --backend ninja --wrap-mode nodownload --native /home/dg0yt/Projekte/vcpkg/vcpkg/buildtrees/fontconfig/meson-native-x64-linux.log --default-library static --libdir lib --native /home/dg0yt/Projekte/vcpkg/vcpkg/buildtrees/fontconfig/meson-native-x64-linux-debug.log -Ddebug=true --prefix /home/dg0yt/Projekte/vcpkg/vcpkg/packages/fontconfig_x64-linux/debug --includedir ../include -Dcmake_prefix_path=['/home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug','/home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux'] /home/dg0yt/Projekte/vcpkg/vcpkg/buildtrees/fontconfig/src/2.13.94-6fd83e1cbe.clean
    Working Directory: /home/dg0yt/Projekte/vcpkg/vcpkg/buildtrees/fontconfig/x64-linux-dbg
    Error code: 2
    See logs for more information:
      /home/dg0yt/Projekte/vcpkg/vcpkg/buildtrees/fontconfig/config-x64-linux-dbg-err.log

On second run, it just builds without meson messages and without error!
So still not idempotent, but in an unexpected way.
It downloads the newer tool in the first run, but use the outdate system version.
Now imagine some other port building with meson before fontconfig, but actually happy with the outdate version. CI passed.

(And I only noticed that because I canceled the first run when I noticed the missing system meson...)

@Neumann-A
Copy link
Contributor

Neumann-A commented Nov 11, 2021

Ah it is probably not unsetting the variable after the first search.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 11, 2021 •
edited
Loading

On the positive side, it is not a regression :-) Now with meson installed, I see the same behaviour in the old tree.

My old tree was not old enough. @Neumann-A is right in #21173 (comment)

@dg0yt
Copy link
Contributor

dg0yt commented Nov 11, 2021

Ah it is probably not unsetting the variable after the first search.

More specific, I guess it is related to ${program} being used both as a cache variable (via find_program) and as a normal variable. Unsetting the normal variable unhides the cached value.

Neumann-A
Neumann-A reviewed Nov 11, 2021
View reviewed changes
scripts/cmake/vcpkg_find_acquire_program.cmake
do_version_check()
set(${VAR}_VERSION_CHECKED ON)
if(NOT ${VAR})
unset(SCRIPT_${VAR} CACHE)
Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

notice: here the script var was unset in the cache not locally due to find_file output being a cache var

@Neumann-A
Copy link
Contributor

Neumann-A commented Nov 11, 2021

On the positive side, it is not a regression :-) Now with meson installed, I see the same behaviour in the old tree.

CI disagrees (the current main run just before these changes were merged).

Starting package 214/1705: tool-meson:x64-linux
Building package tool-meson[core]:x64-linux...
-- Found external python3('Python 3.8.10').
-- Found meson('0.53.2') but at least version 0.58.1 is required! Trying to use internal version if possible!
-- Downloading https://github.com/mesonbuild/meson/archive/aeda7f249c4a5dbbecc52e44f382246a2377b5b0.tar.gz -> meson-aeda7f249c4a5dbbecc52e44f382246a2377b5b0.tar.gz...
-- Using meson: /usr/bin/python3;/mnt/vcpkg-ci/downloads/tools/meson/meson-aeda7f249c4a5dbbecc52e44f382246a2377b5b0/meson.py
-- Performing post-build validation
-- Performing post-build validation done

while now:

Starting package 171/886: tool-meson:x64-linux
Building package tool-meson[core]:x64-linux...
-- Found meson('0.53.2') but at least version 0.58.1 is required! Trying to use internal version if possible!
-- Downloading https://github.com/mesonbuild/meson/archive/aeda7f249c4a5dbbecc52e44f382246a2377b5b0.tar.gz -> meson-aeda7f249c4a5dbbecc52e44f382246a2377b5b0.tar.gz...
-- Using meson: /usr/bin/python3;/usr/bin/meson
-- Performing post-build validation
-- Performing post-build validation done

@Neumann-A
Copy link
Contributor

Neumann-A commented Nov 11, 2021

Fix here #21341

@dg0yt dg0yt mentioned this pull request Nov 12, 2021
Merged
@Cheney-W Cheney-W mentioned this pull request Aug 17, 2022
Merged
@strega-nil strega-nil deleted the vcpkg_find_acquire_program branch March 31, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

+2 more reviewers

@dg0yt dg0yt dg0yt left review comments

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

Reviewers whose approvals may not affect merge requirements

Assignees

@JackBoosY JackBoosY

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@strega-nil-ms @BillyONeal @cenit @Neumann-A @dg0yt @JackBoosY @strega-nil