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_find_acquire_program] add version check for ninja #12895

Merged
merged 9 commits into from
Aug 16, 2020
Merged

[vcpkg_find_acquire_program] add version check for ninja #12895

merged 9 commits into from
Aug 16, 2020

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Aug 13, 2020 •
edited
Loading

closes
#12894
#12888

@Neumann-A Neumann-A mentioned this pull request Aug 13, 2020
Closed
@LilyWangL LilyWangL self-requested a review August 14, 2020 01:49
@LilyWangL LilyWangL self-assigned this Aug 14, 2020
@LilyWangL LilyWangL added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Aug 14, 2020
@LilyWangL LilyWangL mentioned this pull request Aug 14, 2020
Closed
@Neumann-A
Copy link
Contributor Author

hmm i don't get why cmake suddenly fails the compiler identification.

@LilyWangL LilyWangL mentioned this pull request Aug 14, 2020
Closed
@MVoz
Copy link
Contributor

MVoz commented Aug 14, 2020

maybe there is no script import

vcpkg/ports/lapack-reference/portfile.cmake

Line 7 in 0b5bbe3

include(vcpkg_find_fortran)

?
include(${SCRIPTS}/cmake/vcpkg_find_fortran)

@Neumann-A
Copy link
Contributor Author

@voskrese: If that would be the case it wouldn't print the message:
-- No Fortran compiler found on the PATH. Using MinGW gfortran!
Since it does the include is working.

@MVoz
Copy link
Contributor

MVoz commented Aug 14, 2020 •
edited
Loading

strange, it works locally

but there is another error by the way

-- Using source at E:/tools/vcpkg/buildtrees/lapack-reference/src/v3.8.0-954f10683f
-- No Fortran compiler found on the PATH. Using MinGW gfortran!
-- Configuring x64-windows
-- Building x64-windows-dbg
-- Building x64-windows-rel
CMake Error at scripts/cmake/vcpkg_fixup_pkgconfig.cmake:84 (message):
pkg-config error output:Package blas was not found in the pkg-config search
path.

Perhaps you should add the directory containing `blas.pc'

to the PKG_CONFIG_PATH environment variable

Package 'blas', required by 'virtual:world', not found
Call Stack (most recent call first):
scripts/cmake/vcpkg_fixup_pkgconfig.cmake:309 (vcpkg_fixup_pkgconfig_check_files)
ports/lapack-reference/portfile.cmake:94 (vcpkg_fixup_pkgconfig)
scripts/ports.cmake:96 (include)

@Neumann-A
Copy link
Contributor Author

@voskrese What features did you try to install? Never saw that error.

@MVoz
Copy link
Contributor

MVoz commented Aug 14, 2020 •
edited
Loading

vcpkg install lapack-reference[core] --triplet x64-windows

lapack-reference_x64-windows>tree /F

├───bin
│ libblas.dll
│ liblapack.dll
│
├───debug
│ ├───bin
│ │ libblas.dll
│ │ liblapack.dll
│ │
│ └───lib
│ │ libblas.dll.a
│ │ libblas.lib
│ │ liblapack.dll.a
│ │ liblapack.lib
│ │
│ └───pkgconfig
│ blas.pc
│ lapack.pc
│
├───lib
│ │ libblas.dll.a
│ │ libblas.lib
│ │ liblapack.dll.a
│ │ liblapack.lib
│ │
│ └───pkgconfig
│ blas.pc
│ lapack.pc
│
└───share
└───lapack-reference
lapack-config-version.cmake
lapack-config.cmake
lapack-targets-debug.cmake
lapack-targets-release.cmake
lapack-targets.cmake
vcpkg_abi_info.txt

@Neumann-A
Copy link
Contributor Author

Package 'blas', required by 'virtual:world', not found

wonder what virtual:world means. Normally it should print a .pc file name. like lapack.pc. Maybe your pkg-config is somehow broken?

@MVoz
Copy link
Contributor

MVoz commented Aug 14, 2020

maybe, maybe not

time will show on other users

there is no time to look for the reason yet

@MVoz
Copy link
Contributor

MVoz commented Aug 14, 2020

-- [DEBUG] Using pkg-config from: E:/tools/vcpkg/downloads/tools/msys2/msys64/mingw64/bin/pkg-config.exe
-- [DEBUG] Files: E:/tools/vcpkg/packages/lapack-reference_x64-windows/lib/pkgconfig/blas.pc;E:/tools/vcpkg/packages/lapack-reference_x64-windows/lib/pkgconfig/lapack.pc
-- [DEBUG] Checking package (RELEASE): blas
CMake Error at scripts/cmake/vcpkg_fixup_pkgconfig.cmake:84 (message):
pkg-config error output:Package blas was not found in the pkg-config search
path.

Perhaps you should add the directory containing `blas.pc'

separates **;** -- blas.pc;E:/tools

@Neumann-A
Copy link
Contributor Author

@voskrese: That is ok because it is a cmake list for the foreach loop

@MVoz
Copy link
Contributor

MVoz commented Aug 14, 2020

then where is not normal?))) updated it now (vcpkg)

@Neumann-A
Copy link
Contributor Author

Neumann-A commented Aug 14, 2020 •
edited
Loading

@voskrese: Found it:
E:/tools/vcpkg/downloads/tools/msys2/msys64/mingw64/bin/pkg-config.exe
is windows nativ pkg-config.
So probably setting PKG_CONFIG_PATH with : does not work for you. (and probably also the path conversions.)
The current script should find the msys version and not the mingw version.
So you probably applied some changes ontop current master

@MVoz
Copy link
Contributor

MVoz commented Aug 14, 2020 •
edited
Loading

probably figured out why
before that by the way there were no mistakes

E:/tools/vcpkg/downloads/tools/msys2/msys64/mingw64/bin/pkg-config.exe --version

1.6.3

installed from mingw I'll try again

E:/tools/vcpkg/downloads/tools/msys2/msys64/mingw64/bin/pkg-config.exe --version

0.29.2

@Neumann-A
Copy link
Contributor Author

Version does not matter if the path handling of pkg-config is vastly different
the one in <msysroot>/usr/bin uses linux style path handling while
the one in <msysroot>/mingw64/bin uses windows style path handling.
Currently the script is expecting linux style paths which will be switched by #12626

@Neumann-A
Copy link
Contributor Author

Shiva regression (baseline error):

CMake Error at modules/lua/CMakeLists.txt:14 (configure_file):
  configure_file Problem configuring file

@Neumann-A Neumann-A mentioned this pull request Aug 15, 2020
Merged
@ras0219
Copy link
Contributor

ras0219 commented Aug 15, 2020

The shiva issue is 90% probably due to racy writes back to the source dir -- this can be fixed with NO_PARALLEL_CONFIGURE.

@Neumann-A
Copy link
Contributor Author

@ras0219: I know but the fix for that should be a separate PR.

This was referenced Aug 16, 2020
Merged
Closed
@MVoz
Copy link
Contributor

MVoz commented Aug 16, 2020

@LilyWangL

if everything is fine, combine
mistakes need to be corrected

@strega-nil strega-nil merged commit 8800ba9 into microsoft:master Aug 16, 2020
@Neumann-A Neumann-A deleted the ninja-version-check branch August 16, 2020 20:52
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Aug 17, 2020
@Jimmy-Hu
Merge pull request #285 from microsoft/master
3d62693 
[vcpkg_find_acquire_program] add version check for ninja (microsoft#12895)
remz1337 pushed a commit to remz1337/vcpkg that referenced this pull request Aug 23, 2020
@Neumann-A @c72578
@ras0219 @remz1337
[vcpkg_find_acquire_program] add version check for ninja (microsoft#1…
3fc1c2f 
…2895)

* [vcpkg_find_acquire_program] add version check for ninja

* Change VERSION to NINJA_VERSION

* Missed VERSION change

* some more corrections

* add missing PACKAGES parameter

* add osx hash

Co-authored-by: Wolfgang Stöggl <[email protected]>

* Remove apt/brew package names

Co-authored-by: ras0219 <[email protected]>

* move supported around and disable it for freebsd

* fix small command hickup which does not matter for ninja

Co-authored-by: Wolfgang Stöggl <[email protected]>
Co-authored-by: ras0219 <[email protected]>
@lebdron lebdron mentioned this pull request Jan 8, 2021
Merged
JackBoosY
JackBoosY reviewed Jul 27, 2021
View reviewed changes
scripts/cmake/vcpkg_find_acquire_program.cmake
string(STRIP "${${VAR}_VERSION_OUTPUT}" ${VAR}_VERSION_OUTPUT)
#TODO: REGEX MATCH case for more complex cases!
if(NOT ${VAR}_VERSION_OUTPUT VERSION_GREATER_EQUAL ${VAR}_VERSION)
message(STATUS "Found ${PROGNAME}('${${VAR}_VERSION_OUTPUT}') but at least version ${${VAR}_VERSION} is required! Trying to use internal version if possible!")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we ask for the lowest version but do not match, there may be a problem when using the lower version of the tool in the system.
Related: #18955 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a VERSION_GREATER_EQUAL check. So if meson on the system is equal or greater it will be used. Otherwise it should use the vcpkg version. This is the same logic vcpkg applies to cmake. If the system CMake version is greater or equal to the minimum set by vcpkg it will be used. Otherwise vcpkg will download its own cmake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c72578 c72578 c72578 left review comments

@ras0219 ras0219 ras0219 left review comments

@JackBoosY JackBoosY JackBoosY left review comments

@LilyWangL LilyWangL Awaiting requested review from LilyWangL

Assignees

@JackBoosY JackBoosY

@LilyWangL LilyWangL

Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Neumann-A @MVoz @ras0219 @c72578 @JackBoosY @strega-nil @LilyWangL