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

[vcpkg_cmake_config_test] Add function to test the exported cmake configurations #15173

Closed

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Dec 17, 2020

This function will replace vcpkg_test_cmake, have the following features:

Usage:

vcpkg_cmake_config_test(
    USAGE        Curl CONFIG
    TARGET_NAMES CURL::libcurl
    HEADERS      curl/curl.h
    FUNCTIONS    curl_global_init
)
  • Use vcpkg tool chain
  • Obtain targets automatically
  • Automatically disabled in non-debug mode
  • Check all targets
  • Check the header file path
  • Check function export

To test the header file path and function symbols, we can create vcpkg-test.cpp/vcpkg-test.c in PORT_DIR and add test code to it. Example:
vcpkg-test.cpp:

#include <stdio.h>
#include <curl/curl.h>

int main()
{
    return curl_global_init();
}

For further testing of cmake export variables, we can also create vcpkg-test.cmake in PORT_DIR and write test cmake code.

For exmaple:
Add vcpkg_cmake_config_test() to ports/cjson/portfile.cmake, install cjson with --debug, the expected error will shows (known bug):

Make Error at installed/x64-windows/share/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake:167 (message):
    Test cmake configuration failed, please check cmake configuration file!
    Error code: 1
    See logs for more information:
        G:/15173/vcpkg/buildtrees/cjson/config-test-cmake-debug-out.log
        G:/15173/vcpkg/buildtrees/cjson/config-test-cmake-debug-err.log

log details:

CMake Error at G:/15173/vcpkg/packages/cjson_x86-windows/share/cjson/cjson.cmake:68 (message):
  The imported target "cjson" references the file

     "/debug/lib/cjson.lib"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "G:/15173/vcpkg/packages/cjson_x86-windows/share/cjson/cjson.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  G:/15173/vcpkg/packages/cjson_x86-windows/share/cjson/cJSONConfig.cmake:14 (include)
  G:/15173/vcpkg/scripts/buildsystems/vcpkg.cmake:788 (_find_package)
  CMakeLists.txt:4 (find_package)

@JackBoosY JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal This PR or Issue was filed by the vcpkg team. labels Dec 17, 2020
@JackBoosY
Copy link
Contributor Author

@BillyONeal @strega-nil Any suggestions are good.

@strega-nil
Copy link
Contributor

I do like this feature a lot. I'm not sure what suggestions you're looking for?

@JackBoosY
Copy link
Contributor Author

@strega-nil I think this type of testing should only be carried out in two situations:

  1. editable mode
  2. CI mode

Most post checks should follow this policy, because for users, they have been tested in the PRs before being merged into the master, so there is no need to waste more time.

@JackBoosY
Copy link
Contributor Author

Close this PR temporary.

@JackBoosY JackBoosY closed this Feb 7, 2021
@JackBoosY JackBoosY reopened this May 25, 2021
@JackBoosY
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@dg0yt
Copy link
Contributor

dg0yt commented May 25, 2021

Will this be move to a new port, or merged into an existing vcpkg-cmake... port? I suppose it would also shorten CI runs to not touch ports.cmake.

@JackBoosY
Copy link
Contributor Author

@strega-nil I think all the ports whose called vcpkg_fixup_cmake_targets or vcpkg_cmake_config_fixup should automaticly call this function. What do you think about?

@dg0yt
Copy link
Contributor

dg0yt commented May 28, 2021

all the ports whose called vcpkg_fixup_cmake_targets or vcpkg_cmake_config_fixup should automaticly call this function.

Can you perhaps test with port geos? I added a usage file recently because the default instructions were somewhat misleading. And now I wonder if automated testing might fail to identify the canonical use in a similar way.

@JackBoosY
Copy link
Contributor Author

@dg0yt No errors.
This function will only automatically detect the generated cmake configuration file, and cannot identify the content in the usage and test it.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Oct 26, 2021

@dg0yt Please note we should test the following contents:

  1. cmake targets.
  2. cmake macros provided by *-[Cc]onfig.cmake.
  3. include headers (test the path).
  4. function symbols.

And I noticed you've done only part of them.

For more, we also should allow the maintainer to provide a custom test code to test the binary.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 26, 2021

@dg0yt Please note we should test the following contents:

  1. cmake targets.
  2. cmake macros provided by *-[Cc]onfig.cmake.
  3. include headers (test the path).
  4. function symbols.

And I noticed you've done only part of them.

Yes, because I had a different focus (older versions of cmake). So apart from the pure find_package it is more a complementary effort than an overlap.

But given your function's arguments, why not cover find modules? Do you need a proof-of-concept?

For more, we also should allow the maintainer to provide a custom test code to test the binary.

I agree.

@JackBoosY
Copy link
Contributor Author

But given your function's arguments, why not cover find modules? Do you need a proof-of-concept?

Because some ports do not actively provide the find modules and use cmake one, we must add additional parameters to achieve this.
This PR is intended to check all the generated files, especially the generated cmake configuration files.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 27, 2021

Because some ports do not actively provide the find modules and use cmake one, we must add additional parameters to achieve this.

  • Many ports provide wrappers which need to be tested and which can be discovered.
  • You cannot completely rely on auto-discovery anyway (mostly due to handling of upper vs. lower case).
  • The existing parameters are almost okay.
    • What doesn't match is CONFIG_NAME. There could be an MODULE_NAME instead.
    • In absence of a TARGET_NAME, the build should use the <MODULE_NAME>_... variables.

There could also be a unified signature like this:

vcpkg_cmake_package_test(
    USAGE Curl CONFIG
    TARGET_NAMES CURL::libcurl
    HEADERS     curl/curl.h
    FUNCTIONS   curl_global_init
)
vcpkg_cmake_package_test(
    USAGE Curl
    TARGET_NAMES CURL::libcurl
    HEADERS     curl/curl.h
    FUNCTIONS   curl_global_init
)

where USAGE is passed to the find_package call literally.

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 feel like this needs a clearer separation of package name and folder name.

endif()

if (arg_CONFIG_NAME)
set(target_folder "${arg_CONFIG_NAME}")
Copy link
Contributor

Choose a reason for hiding this comment

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

A config for Pkg can be found in different target folders:

  • Pkg (as Pkg/PkgConfig.cmake)
  • pkg (as pkg/pkg-config.cmake)

But a config for Pkg is different from a config for pkg. (Side effects: setting Pkg_FOUND, Pkg_LIBRARIES etc.)

Copy link
Contributor Author

@JackBoosY JackBoosY Oct 27, 2021

Choose a reason for hiding this comment

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

  1. For CONFIG mode, we should use the target name, now I think it's correct.
  2. For MODULE mode, we need to use the Find module provided cmake macros, we still need to extend this function to add additional option TARGET_LIBRARIES, TARGET_INCLUDES, TARGET_DEFINITIONS and TARGET_OPTIONS. Also, we need to consider whether these options are just "extend" cmake test code or "replace" cmake test code. That's why I set up the ability to use the extended cmake test code.

ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 078f3e51efd5ea6072d0ba28d11baa946519a2ce -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/c-/curl.json b/versions/c-/curl.json
index 5ba5d000..335aaebe 100644
--- a/versions/c-/curl.json
+++ b/versions/c-/curl.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "b43d25f9b63c3be9d9b67ef81cb4cfc18c75a848",
+      "git-tree": "a6939590c8a4af6b772b6a0a40bf107451e9475e",
       "version": "7.79.1",
       "port-version": 1
     },
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 801f78c0..3ae01541 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "4d873c8d4d63892838c407e1b7eb1c297377a8b6",
+      "git-tree": "da185d09123093850dd27c8775df33c20df88ce3",
       "version-date": "2021-10-19",
       "port-version": 0
     },

Comment on lines +226 to +228
set(extern_header_checks "")
set(extern_symbol_checks_base "")
set(extern_symbol_check_symbol "")
Copy link
Contributor Author

@JackBoosY JackBoosY Oct 27, 2021

Choose a reason for hiding this comment

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

These macros are strings, so we shouldn't use vcpkg_list.

@JackBoosY
Copy link
Contributor Author

Ping @dg0yt for review again.

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.

Now here is a lot more.
Disclaimer: I'm not a native English speaker.

ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
list(LENGTH target_folders folder_size)

if (target_size EQUAL 0 OR folder_size EQUAL 0)
message(WARANING "Found the following cmake configuration file but not found the target.")
Copy link
Contributor

Choose a reason for hiding this comment

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

WARNING
... but did not find the target.

Note that the warning text doesn't match the case of folder_size == 0.

I keep wondering if it would be better to start with a glob for *-config.cmake *Config.cmake (otherwise: error "No config file found"), generate the list of package names and directories from this config file list, and only then start collecting targets from these directories (otherwise: error "No targets found for config Xyz").

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 for installing other cmake files instead of cmake configuration entry files.
Please note that the *Cconfig.cmake file does not provide add_library in most cases.

endif()

if (NOT folder_size EQUAL 1 AND NOT arg_USAGE)
message(FATAL_ERROR "More than one folder contains cmake configuration files, please set \"TARGET_NAMES\" to select the certain target name")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an example case? I don't think this can be assumed to be an error. There can be folder PROJ4 providing PROJ4::PROJ4 (legacy), and there can be folder PROJ, providing PROJ::PROJ (modern).
But probably it is a limitation of the current implementation which would be resolved by splitting identifying folders from processing folders, instead of doing this simultaneously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some ports, the upstream provides many targets, and these targets independently generate the corresponding targets.cmake file. These files may be installed in different folders (named after the target name).
We should avoid this situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your are talking about deep trees within a single package. Here you statement makes sense.
But my focus was separate cmake packages.
This cannot be improved while iterating simultaneously over multiple cmake packages. In my previous comment, I should have used the words "splitting identifying package names from ...". Or better write pseudo code:

  • Collect package names by non-recursive looking for */*Config.cmake */*-config.cmake.
    (This step could be skipped in case of arg_USAGE.)
  • For each package name, collect targets from the cmake files (maybe recursively).
    (This step could be skipped in case of arg_TARGET_NAMES, assuming a single package name.)

ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
endmacro()

function(vcpkg_cmake_config_test)
if (NOT _VCPKG_EDITABLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is the right indicator for controlling the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect this test to run in edit mode or CI mode to reduce user build time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the argument about build time, But edit mode is somewhat exceptional in my personal use, and it is propably inactive during CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope the ci command will pass a special VCPKG_ variable to cmake to enable this function.
But for now I think it can be enabled in edit mode first.

ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 078f3e51efd5ea6072d0ba28d11baa946519a2ce -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/c-/curl.json b/versions/c-/curl.json
index 5ba5d000..335aaebe 100644
--- a/versions/c-/curl.json
+++ b/versions/c-/curl.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "b43d25f9b63c3be9d9b67ef81cb4cfc18c75a848",
+      "git-tree": "a6939590c8a4af6b772b6a0a40bf107451e9475e",
       "version": "7.79.1",
       "port-version": 1
     },
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 801f78c0..cd4fcce4 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "4d873c8d4d63892838c407e1b7eb1c297377a8b6",
+      "git-tree": "b53769d6f8e96ae25f0f8d619cf97e08ad270120",
       "version-date": "2021-10-19",
       "port-version": 0
     },

ports/vcpkg-cmake-config/vcpkg_cmake_config_test.cmake Outdated Show resolved Hide resolved
return()
endif()

if (NOT folder_size EQUAL 1 AND NOT arg_USAGE)
Copy link
Contributor

Choose a reason for hiding this comment

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

ATM this function is called only for NOT arg_USAGE.

You might actually make use of arg_USAGE, by picking out the package name Pkg (first item) and using that to limit the globbing to Pkg/PkgConfig.cmake and pkg/pkg-config.cmake. But this is not urgent as for now arg_USAGE implies arg_TARGET_NAMES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please describe more?
As I know, some ports have generated pkg[Cc]onfig.cmake files corresponding to different targets, so here we should prompt the maintainer for the usage.

endif()

if (NOT folder_size EQUAL 1 AND NOT arg_USAGE)
message(FATAL_ERROR "More than one folder contains cmake configuration files, please set \"TARGET_NAMES\" to select the certain target name")
Copy link
Contributor

Choose a reason for hiding this comment

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

Your are talking about deep trees within a single package. Here you statement makes sense.
But my focus was separate cmake packages.
This cannot be improved while iterating simultaneously over multiple cmake packages. In my previous comment, I should have used the words "splitting identifying package names from ...". Or better write pseudo code:

  • Collect package names by non-recursive looking for */*Config.cmake */*-config.cmake.
    (This step could be skipped in case of arg_USAGE.)
  • For each package name, collect targets from the cmake files (maybe recursively).
    (This step could be skipped in case of arg_TARGET_NAMES, assuming a single package name.)

endmacro()

function(vcpkg_cmake_config_test)
if (NOT _VCPKG_EDITABLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the argument about build time, But edit mode is somewhat exceptional in my personal use, and it is propably inactive during CI.

endforeach()
endif()

foreach(target_name IN LISTS target_names)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's the loop which is actually limiting us to the single cmake package case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, but it doesn't matter, this function will only be enabled in the official triplet which doesn't include the single configuration triplet yet.

@PhoebeHui PhoebeHui marked this pull request as draft December 2, 2021 06:14
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout a91bc0c2d4af9a9e521cda0e39a1f108a8d17def -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 24c52c1f..f00e2849 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -1702,7 +1702,7 @@
     },
     "curl": {
       "baseline": "7.80.0",
-      "port-version": 0
+      "port-version": 1
     },
     "curlpp": {
       "baseline": "2018-06-15",
@@ -7105,7 +7105,7 @@
       "port-version": 0
     },
     "vcpkg-cmake-config": {
-      "baseline": "2021-12-01",
+      "baseline": "2021-12-17",
       "port-version": 0
     },
     "vcpkg-gfortran": {
diff --git a/versions/c-/curl.json b/versions/c-/curl.json
index a73767ce..97a8f51b 100644
--- a/versions/c-/curl.json
+++ b/versions/c-/curl.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "847baeeb1e54716212cb71f724be3c0df71a4f43",
+      "version": "7.80.0",
+      "port-version": 1
+    },
     {
       "git-tree": "8e13da05c975cb6f5bed6cf3b8054a817a00b45d",
       "version": "7.80.0",
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index dda29410..de6de278 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "ca4fd79144060953c1a7ec689ccf3feb18b98650",
+      "version-date": "2021-12-17",
+      "port-version": 0
+    },
     {
       "git-tree": "51df1bbddb22782b9e7f23f9b3588674184e991a",
       "version-date": "2021-12-01",

@JackBoosY JackBoosY marked this pull request as ready for review December 17, 2021 08:48
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout a91bc0c2d4af9a9e521cda0e39a1f108a8d17def -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 68bb36ba..02bd3657 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -1702,7 +1702,7 @@
     },
     "curl": {
       "baseline": "7.80.0",
-      "port-version": 0
+      "port-version": 1
     },
     "curlpp": {
       "baseline": "2018-06-15",
@@ -7109,7 +7109,7 @@
       "port-version": 0
     },
     "vcpkg-cmake-config": {
-      "baseline": "2021-12-01",
+      "baseline": "2021-12-17",
       "port-version": 0
     },
     "vcpkg-gfortran": {
diff --git a/versions/c-/curl.json b/versions/c-/curl.json
index a73767ce..97a8f51b 100644
--- a/versions/c-/curl.json
+++ b/versions/c-/curl.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "847baeeb1e54716212cb71f724be3c0df71a4f43",
+      "version": "7.80.0",
+      "port-version": 1
+    },
     {
       "git-tree": "8e13da05c975cb6f5bed6cf3b8054a817a00b45d",
       "version": "7.80.0",
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index dda29410..a9a78510 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "d6b915f72cc87630c9bd0476314942ef17b2ba44",
+      "version-date": "2021-12-17",
+      "port-version": 0
+    },
     {
       "git-tree": "51df1bbddb22782b9e7f23f9b3588674184e991a",
       "version-date": "2021-12-01",

@dg0yt
Copy link
Contributor

dg0yt commented Dec 24, 2021

I experiment with a modified variant of this PR which also covers testing CMake Find modules (eventually replacing the cmake-user test port). I think I can turn this into a proper PR in the next days.

With regard to control over enabling certain tests, I would suggest to use a directory like share/vcpkg-tests/enabled/. Tests might be enabled by manual install commands on the maintainer's workstation (interactive), or by directly copying files from vcpkg repo to the installed tree on CI systems (non-interactively). This avoids build time impact on normal users (e.g. for testing with CMake 3.4), and it also decouples user artifacts from rebuilds due to test improvements. And no change to the tool binary is needed.

@JackBoosY
Copy link
Contributor Author

In favor of #22258.

@JackBoosY JackBoosY closed this Jan 30, 2022
@PhoebeHui PhoebeHui removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants