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

Make sure vcpkg_test_cmake uses the correct generator #5635

Closed
wants to merge 2 commits into from

Conversation

tarcila
Copy link
Contributor

@tarcila tarcila commented Mar 11, 2019

Otherwise, on Windows, it will default to x86 based generator that will
fail finding x64 builds. For instance, when building for x64-windows:

CMake Error at CMakeLists.txt:4 (find_package):
Could not find a configuration file for package "CapnProto" that is
compatible with requested version "".

The following configuration files were considered but not accepted:

vcpkg/packages/capnproto_x64-windows/share/capnproto/CapnProtoConfig.cmake, version: 0.7.0 (64bit)

@vicroms vicroms self-assigned this Mar 11, 2019
set(GENERATOR ${_tc_GENERATOR})
else()
# vcpkg_configure_cmake defines _VCPKG_CMAKE_GENERATOR when called
set(GENERATOR ${_VCPKG_CMAKE_GENERATOR})
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing:
${_VCPKG_CMAKE_GENERATOR} might not be set.

Changes are similar to:
4cfca10
and
be85bc5

except that you allow to manually define a generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that's why the content of GENERATOR is tested later on.
I'll update the comment to make this more explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Quote from a discussion about this with ras0219:

We should always use ninja here (and skip it for 32-bit hosts). There's no reason to use the MSBuild generator, nor for it to be configurable. This is because all generators should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look at this PR!

I agree with the latest part of the comment. But I don't get the rational for the very first part: why skip 32bits? and why ninja only?

The very first intent of my commit is, by using the same generator, that we target the same platform/architecture: the breakage on my side is that the package is generated for x64 but is searched for by an x86 generator.
But indeed, reusing the same generator might not be the way to go. Maybe just enforcing the architecture could be sufficient first step (I don't know if ninja generator supports -A flag).

Note that in a general case, it seems to me that find_package logic will be mostly independent of the generator used (when try_compile/link/run are not used at least).

@@ -4,7 +4,7 @@
##
## ## Usage:
## ```cmake
## vcpkg_test_cmake(PACKAGE_NAME <name> [MODULE])
## vcpkg_test_cmake(PACKAGE_NAME <name> [MODULE] [GENERATOR generator])
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if targets could also be tested

@vicroms
Copy link
Member

vicroms commented Mar 14, 2019

Hi @tarcila

Thanks for your contribution!

I'll run a test against all packages, as it looks like this PR slightly changes the default behavior of calling vcpkg_test_cmake(PACKAGE_NAME <name>).

Our vcpkg_test_cmake() function has been working improperly for a while.

vcpkg_test_cmake() aims to test CMake integration as it would be consumed by a vcpkg user. but the design is inherently flawed and doesn't reflect a real-world usage scenario. And that's the reason why we don't check or encourage its use.

We are aware of the design flaws in it and we are planning on designing a better CMake integration test mechanism in the future.

@tarcila
Copy link
Contributor Author

tarcila commented Mar 15, 2019

I'll run a test against all packages, as it looks like this PR slightly changes the default behavior of calling vcpkg_test_cmake(PACKAGE_NAME <name>).

I do confirm that the default behavior is changed when the package is generated using vcpkg_configure_cmake(). The rest should behave the same.
I did test a few packages that try and rely on vcpkg_test_cmake(), without regression, but indeed, I am not able to build all the packages on all the architectures, so your help is more than welcome.

Our vcpkg_test_cmake() function has been working improperly for a while.

vcpkg_test_cmake() aims to test CMake integration as it would be consumed by a vcpkg user. but the design is inherently flawed and doesn't reflect a real-world usage scenario. And that's the reason why we don't check or encourage its use.

Noted. I'll drop vcpkg_test_cmake() support from my other PR waiting for a resolution for this problem.
Thanks for the update.

@vicroms
Copy link
Member

vicroms commented Mar 15, 2019

Looks like a similar change sneaked its way throug in PR #5574

Can you solve the conflicts and add the changes to manually specify the generator?

@tarcila
Copy link
Contributor Author

tarcila commented Mar 15, 2019 via email

tarcila added 2 commits March 15, 2019 20:47
Otherwise, on Windows, it will default to x86 based generator that will
fail finding x64 builds. For instance, when building for x64-windows:

CMake Error at CMakeLists.txt:4 (find_package):
  Could not find a configuration file for package "CapnProto" that is
  compatible with requested version "".

  The following configuration files were considered but not accepted:

    vcpkg/packages/capnproto_x64-windows/share/capnproto/CapnProtoConfig.cmake, version: 0.7.0 (64bit)
@tarcila tarcila force-pushed the fix-vcpkg-test-cmake branch from 45945e6 to 08a3951 Compare March 16, 2019 01:00
@tarcila
Copy link
Contributor Author

tarcila commented Mar 16, 2019

@vicroms I have rebased the PR on top of the latest master.

tarcila added a commit to tarcila/vcpkg that referenced this pull request Mar 22, 2019
Integration test is failing for now because of microsoft#5630 and microsoft#5635
This at least makes the package function on Windows x86 and x64.
@vicroms
Copy link
Member

vicroms commented May 16, 2019

Due to multiple issues with vcpkg_test_cmake() it has been disabled.

A new CMake integration test solution will be designed, but for the time being that's still in our backlog.
We'll take the input in this PR and several others into account when designing the new solution.

@vicroms vicroms closed this May 16, 2019
@tarcila tarcila deleted the fix-vcpkg-test-cmake branch May 16, 2019 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants