[vcpkg-cmake-config] Merge INTERFACE_LINK_LIBRARIES configurations #22546

dg0yt · 2022-01-15T22:10:45Z

What does your PR fix?

Fixes direct references in exported cmake config from INTERFACE_LINK_LIBRARIES to binary objects in the release tree, affecting debug usage.
Example: INTERFACE_LINK_LIBRARIES of libwebsockets:x64-linux.
Before this change:

${VCPKG_IMPORT_PREFIX}/lib/libssl.a
${VCPKG_IMPORT_PREFIX}/lib/libcrypto.a
-lpthread
dl
dl
-lpthread
ssl
crypto
/usr/lib/x86_64-linux-gnu/libcap.so
\$<\$<NOT:\$<CONFIG:DEBUG>>:${VCPKG_IMPORT_PREFIX}/lib/libz.a>
\$<\$<CONFIG:DEBUG>:${VCPKG_IMPORT_PREFIX}/debug/lib/libz.a>
${VCPKG_IMPORT_PREFIX}/lib/liblibuv.a

After this change:

\$<\$<NOT:\$<CONFIG:DEBUG>>:${VCPKG_IMPORT_PREFIX}/lib/libssl.a>
\$<\$<CONFIG:DEBUG>:${VCPKG_IMPORT_PREFIX}/debug/lib/libssl.a>
\$<\$<NOT:\$<CONFIG:DEBUG>>:${VCPKG_IMPORT_PREFIX}/lib/libcrypto.a>
\$<\$<CONFIG:DEBUG>:${VCPKG_IMPORT_PREFIX}/debug/lib/libcrypto.a>
-lpthread
dl
dl
-lpthread
ssl
crypto
/usr/lib/x86_64-linux-gnu/libcap.so
\$<\$<NOT:\$<CONFIG:DEBUG>>:${VCPKG_IMPORT_PREFIX}/lib/libz.a>
\$<\$<CONFIG:DEBUG>:${VCPKG_IMPORT_PREFIX}/debug/lib/libz.a>
\$<\$<NOT:\$<CONFIG:DEBUG>>:${VCPKG_IMPORT_PREFIX}/lib/liblibuv.a>
\$<\$<CONFIG:DEBUG>:${VCPKG_IMPORT_PREFIX}/debug/lib/liblibuv.a>

Which triplets are supported/not supported? Have you updated the CI baseline?

all, no
Does your PR follow the maintainer guide?

yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

no: probably to be combined with other PRs.

Direct references to binaries must match the active build type when used.

github-actions

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 bba8794b82a0f44b8709f294850a2c8e96b61415 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 6b49bfc..a5c17f6 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7157,7 +7157,7 @@
       "port-version": 0
     },
     "vcpkg-cmake-config": {
-      "baseline": "2021-12-28",
+      "baseline": "2022-01-15",
       "port-version": 0
     },
     "vcpkg-gfortran": {
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index c0cfef2..0f2b443 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "ad6139358631fba00d80023ce72387703c2348ef",
+      "version-date": "2022-01-15",
+      "port-version": 0
+    },
     {
       "git-tree": "e33152002c946b93a0262931ba8bf54a2e6ab9ad",
       "version-date": "2021-12-28",

JackBoosY · 2022-01-17T00:49:46Z

Can you please merge changes in #21763 to this PR?
@horenmar Can we do that?

dg0yt · 2022-01-17T06:09:58Z

Can you please merge changes in #21763 to this PR?

As soon as these changes are approved.

dg0yt · 2022-01-17T07:36:36Z

The aws-c-common error is an issue with repeated calls of this function: The second call processes all cmake files again, but doesn't have the debug files from the first call.

horenmar · 2022-01-17T11:32:13Z

@horenmar Can we do that?

Sure

github-actions

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 bba8794b82a0f44b8709f294850a2c8e96b61415 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 6b49bfc..a5c17f6 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7157,7 +7157,7 @@
       "port-version": 0
     },
     "vcpkg-cmake-config": {
-      "baseline": "2021-12-28",
+      "baseline": "2022-01-15",
       "port-version": 0
     },
     "vcpkg-gfortran": {
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index c0cfef2..995181e 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "003440e4640da05ce8d7bd49f4028a5c69a39212",
+      "version-date": "2022-01-15",
+      "port-version": 0
+    },
     {
       "git-tree": "e33152002c946b93a0262931ba8bf54a2e6ab9ad",
       "version-date": "2021-12-28",

dg0yt · 2022-01-18T11:30:30Z

Can you please merge changes in #21763 to this PR? @horenmar Can we do that?

#21763 changes a different port. So merging that PR here isn't appropriate if it doesn't qualify for #21415 (comment).

dg0yt · 2022-01-18T18:29:59Z

The xzing-cpp error on linux seems to be a missing -lgomp in linking the zxing-cv executable with opencv4 with feature openmp enabled. At the moment it is not clear to me if this related to this PR, or just another bug uncovered by this particular build. I hope I can build xzing-cpp with opencv4[core,openmp] locally.

Apart from this, CI looks good. The ultimate test would be seing most of the 300 warnings added by #21415 being resolved by this PR (after solving the merge conflict). After all, these are serious issues for the debug builds.

dg0yt · 2022-01-19T08:07:32Z

The xzing-cpp error on linux seems to be a missing -lgomp in linking the zxing-cv executable with opencv4 with feature openmp enabled. At the moment it is not clear to me if this related to this PR, or just another bug uncovered by this particular build. I hope I can build xzing-cpp with opencv4[core,openmp] locally.

I had to learn that this was indeed caused by this PR. This is the downside of strict text matching, and there might have been additional hidden cases. But now it should be fixed in a robust way.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/tmx/vcpkg.json
ports/vcpkg-cmake-config/vcpkg.json
scripts/test_ports/unit-test-cmake/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

strega-nil

Apparently I didn't click "submit review".

ports/vcpkg-cmake-config/vcpkg_cmake_config_fixup.cmake

Co-authored-by: nicole mazzuca <[email protected]>

github-actions

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 bba8794b82a0f44b8709f294850a2c8e96b61415 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 8f1a2f9..4558dde 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "342ab2e2d183e3e962c7a52802085a47f0992d81",
+      "git-tree": "a9ca96f97d8ec147035515a8317d2ce0e06b85c7",
       "version-date": "2022-01-15",
       "port-version": 0
     },

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/tmx/vcpkg.json
ports/vcpkg-cmake-config/vcpkg.json
scripts/test_ports/unit-test-cmake/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/tmx/vcpkg.json
ports/vcpkg-cmake-config/vcpkg.json
scripts/test_ports/unit-test-cmake/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

strega-nil · 2022-01-27T19:23:00Z

Something @ras0219-msft pointed out: It would probably be good to stop parsing CMake with regex, which we could do by instead taking the INTERFACE_LINK_LIBRARIES and wrapping the entire thing in a $<CONFIG:DEBUG> block.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/tmx/vcpkg.json
ports/vcpkg-cmake-config/vcpkg.json
scripts/test_ports/unit-test-cmake/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

strega-nil · 2022-01-27T20:19:02Z

We talked about this at the meeting, and we like the general PR; however, we were wondering if it would be possible to instead wrap the entire lists in $<CONFIG:Debug> or w/e, i.e.:

    INTERFACE_LINK_LIBRARIES "$<$<CONFIG:Debug>:${debug-libraries}>;$<$<NOT<CONFIG:Debug>>:${release-libraries}>"

this would result in uglier files, but would also mean we don't have to parse CMake with regex.

dg0yt · 2022-01-27T20:23:51Z

Something @ras0219-msft pointed out: It would probably be good to stop parsing CMake with regex, which we could do by instead taking the INTERFACE_LINK_LIBRARIES and wrapping the entire thing in a $<CONFIG:DEBUG> block.

How do you suggest to find "the entire thing", inside a cmake command which may also set other properties?
And how to deal with the fact that INTERFACE_LINK_LIBRARIES is a list, not something which can be wrapped as a whole?
And how to deal with multiple targets?

I suggest you improve incrementally if desired. You only need to edit the installed script, and build aws-c-common (the port which needs the already-fixed handling) and libwebsockets (known bad config).

JackBoosY · 2022-02-06T03:24:05Z

Ping @strega-nil-ms for reply.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/tmx/vcpkg.json
ports/vcpkg-cmake-config/vcpkg.json
scripts/test_ports/unit-test-cmake/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

dg0yt · 2022-02-17T07:43:01Z

Three more weeks with no feedback?
See #21415 for a reminder how many ports are currently broken.
EDIT: Broken by vcpkg cmake config fixup, not upstream.

JackBoosY · 2022-02-17T07:51:25Z

Three more weeks with no feedback? See #21415 for a reminder how many ports are currently broken. EDIT: Broken by vcpkg cmake config fixup, not upstream.

Sorry for late, I will ping Nicole again.

strega-nil-ms

alright; I'm okay with this, but I want @ras0219-msft to say okay.

@dg0yt it is very, very difficult to deal with notifications from this repo; I recommend poking me on Discord or emailing me or something if you want me to take a look immediately

dg0yt · 2022-02-17T18:35:36Z

@dg0yt it is very, very difficult to deal with notifications from this repo; I recommend poking me on Discord or emailing me or something if you want me to take a look immediately

Contributing is very difficult, too, sometimes. And I don't get notifications when you edit the comment, even with the explicit mention.

ras0219-msft

I have small nits like "maybe we could combine the regex searches into 2 instead of 4" and "I'm not 100% confident that this is the right regex" but I agree that the right thing to do at this point is to merge this and iterate on the results.

Thanks for the PR!

dg0yt force-pushed the fixup-link-libraries branch 2 times, most recently from 3d8c4d2 to 6f73fe3 Compare January 15, 2022 22:31

dg0yt added 3 commits January 16, 2022 06:14

Merge INTERFACE_LINK_LIBRARIES configurations

1235225

Direct references to binaries must match the active build type when used.

Update tmx for cmake_config_fixup

6bc0467

Update versions

ea150d5

dg0yt force-pushed the fixup-link-libraries branch from 6f73fe3 to ea150d5 Compare January 16, 2022 05:23

github-actions bot requested changes Jan 16, 2022

View reviewed changes

LilyWangLL self-assigned this Jan 16, 2022

LilyWangLL 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 Jan 16, 2022

LilyWangLL requested a review from JackBoosY January 17, 2022 03:36

LilyWangLL assigned JackBoosY Jan 17, 2022

Prevent repeated fixup of the same file

57aee8a

github-actions bot requested changes Jan 18, 2022

View reviewed changes

dg0yt mentioned this pull request Jan 18, 2022

[aws-c-mqtt] Fix dependencies #22611

Merged

dg0yt mentioned this pull request Jan 18, 2022

[vcpkg-cmake-config] Check for unguarded release lib paths #21415

Draft

dg0yt added 2 commits January 19, 2022 06:19

Don't change line structure

eb990b2

Update versions

f029b55

github-actions bot approved these changes Jan 19, 2022

View reviewed changes

CI

4c1c2fe

github-actions bot approved these changes Jan 21, 2022

View reviewed changes

JackBoosY approved these changes Jan 22, 2022

View reviewed changes

JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 22, 2022

JackBoosY requested a review from strega-nil-ms January 22, 2022 04:11

ras0219-msft added the info:reviewed Pull Request changes follow basic guidelines label Jan 25, 2022

JackBoosY removed the requires:discussion label Jan 25, 2022

dg0yt mentioned this pull request Jan 25, 2022

[vcpkg-cmake] Remove cmake generator support restrictions #22626

Merged

strega-nil suggested changes Jan 26, 2022

View reviewed changes

ports/vcpkg-cmake-config/vcpkg_cmake_config_fixup.cmake Outdated Show resolved Hide resolved

strega-nil-ms added the requires:discussion label Jan 26, 2022

JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Jan 27, 2022

Apply CR suggestion

a6c8af4

Co-authored-by: nicole mazzuca <[email protected]>

github-actions bot requested changes Jan 27, 2022

View reviewed changes

Update versions

5bdc6cf

github-actions bot approved these changes Jan 27, 2022

View reviewed changes

dg0yt added 2 commits January 27, 2022 20:33

Fixup CR suggestion

4a320fa

Update versions

d81b6c7

github-actions bot approved these changes Jan 27, 2022

View reviewed changes

dg0yt added 2 commits February 6, 2022 10:39

Merge remote-tracking branch 'origin/master' into fixup-link-libraries

90e2550

Update versions

6808429

github-actions bot approved these changes Feb 6, 2022

View reviewed changes

strega-nil-ms approved these changes Feb 17, 2022

View reviewed changes

ras0219-msft approved these changes Feb 18, 2022

View reviewed changes

JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:discussion labels Feb 18, 2022

vicroms merged commit 3565cab into microsoft:master Feb 18, 2022

dg0yt deleted the fixup-link-libraries branch February 19, 2022 05:09

JackBoosY mentioned this pull request May 19, 2022

[unit-test-cmake] Fix function name and feature name #24799

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vcpkg-cmake-config] Merge INTERFACE_LINK_LIBRARIES configurations #22546

[vcpkg-cmake-config] Merge INTERFACE_LINK_LIBRARIES configurations #22546

dg0yt commented Jan 15, 2022

github-actions bot left a comment

JackBoosY commented Jan 17, 2022

dg0yt commented Jan 17, 2022

dg0yt commented Jan 17, 2022

horenmar commented Jan 17, 2022

github-actions bot left a comment

dg0yt commented Jan 18, 2022

dg0yt commented Jan 18, 2022

dg0yt commented Jan 19, 2022

github-actions bot left a comment

strega-nil left a comment

github-actions bot left a comment

github-actions bot left a comment

strega-nil commented Jan 27, 2022

github-actions bot left a comment

strega-nil commented Jan 27, 2022 •

edited

Loading

dg0yt commented Jan 27, 2022 •

edited

Loading

JackBoosY commented Feb 6, 2022

github-actions bot left a comment

dg0yt commented Feb 17, 2022 •

edited

Loading

JackBoosY commented Feb 17, 2022

strega-nil-ms left a comment •

edited

Loading

dg0yt commented Feb 17, 2022

ras0219-msft left a comment

[vcpkg-cmake-config] Merge INTERFACE_LINK_LIBRARIES configurations #22546

[vcpkg-cmake-config] Merge INTERFACE_LINK_LIBRARIES configurations #22546

Conversation

dg0yt commented Jan 15, 2022

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

github-actions bot left a comment

Choose a reason for hiding this comment

JackBoosY commented Jan 17, 2022

dg0yt commented Jan 17, 2022

dg0yt commented Jan 17, 2022

horenmar commented Jan 17, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

dg0yt commented Jan 18, 2022

dg0yt commented Jan 18, 2022

dg0yt commented Jan 19, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

strega-nil left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

strega-nil commented Jan 27, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

strega-nil commented Jan 27, 2022 • edited Loading

dg0yt commented Jan 27, 2022 • edited Loading

JackBoosY commented Feb 6, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

dg0yt commented Feb 17, 2022 • edited Loading

JackBoosY commented Feb 17, 2022

strega-nil-ms left a comment • edited Loading

Choose a reason for hiding this comment

dg0yt commented Feb 17, 2022

ras0219-msft left a comment

Choose a reason for hiding this comment

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

strega-nil commented Jan 27, 2022 •

edited

Loading

dg0yt commented Jan 27, 2022 •

edited

Loading

dg0yt commented Feb 17, 2022 •

edited

Loading

strega-nil-ms left a comment •

edited

Loading