[cmake-user] Verify library location for debug vs. release, fix ports by dg0yt · Pull Request #21641 · microsoft/vcpkg

dg0yt · 2021-11-24T07:11:40Z

What does your PR fix?

Extends the user project test port to verify that library locations match the build type.
Always runs tests with the current CMake, in addition to the (optional) minimum supported version. Enables testing of find modules added to CMake after the minimum supported version (e.g. ICU).
Adds find_package testing for ICU, JPEG, TIFF.
~~Modernizes, and adds a wrapper for CMake < 3.7 to, port bzip2.~~
Revises and fixes the wrapper for CMake < 3.7 in port freetype.
Fixes the wrapper in port icu.
Fixes the wrapper for CMake < 3.12 in port libjpeg-turbo.

In general, for old cmake find modules which don't support debug+release in <PKG>_LIBRARY, the wrappers now choose the debug lib for the Debug configuration, and the release lib otherwise. This fixes usage of <PKG>_LIBRARY as a single filepath, such as in the old FindJPEG.cmake.
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?

yes

ports/bzip2/portfile.cmake

dg0yt · 2021-11-29T20:57:01Z

Removed CURL and OpenSSL from this test again. These wrappers seem to need more work, and the PR is big enough.

ports/freetype/vcpkg-cmake-wrapper.cmake

JackBoosY · 2021-11-30T02:38:41Z

I wish other guys to review the test-ports part.

dg0yt · 2021-12-20T07:44:06Z

I would love to use the functions from #21783 for reimplementing the debug/release configuration setup for legacy find modules.

I also learned that I cannot easily move the wrapper tests to the actual ports: During portfile execution, the wrapper is in the staging dir (packages). It would take additional changes to accept a wrapper from the staging dir. OTOH it would allow a consolidatet vcpkg-cmake-validate port for find modules and config files, including advanced tests for symbols.

JackBoosY · 2021-12-21T09:14:11Z

Can you please resolve the file conflicts?

Thanks.

dg0yt · 2022-01-02T09:48:31Z

#22258 could replace cmake-user (after adding it to all ports tested here).

strega-nil-ms

minor changes requested

ports/icu/vcpkg-cmake-wrapper.cmake

scripts/test_ports/cmake-user/project/CMakeLists.txt

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

If you feel able to do so, please consider adding a "license" field to the following files:

ports/freetype/vcpkg.json
ports/icu/vcpkg.json
scripts/test_ports/cmake-user/vcpkg.json

Valid values for the license field can be found in the documentation

dg0yt · 2022-03-29T06:09:50Z

The new Curses error is from looking at <Pkg>_LIBRARIES. It turns out that the Curses module finds the static "curses" lib from the system, but the "form" lib from the installed port ncurses:

-- CURSES_LIBRARIES: /usr/lib/x86_64-linux-gnu/libcurses.so;/mnt/vcpkg-ci/installed/x64-linux/lib/libform.a
-- Configuring incomplete, errors occurred!

(Port ncurses provides a "ncurses" lib, not a "curses" lib.)

There are two aspects:

On Ubuntu, the system lib is from ncurses, so we mix different builds of ncurses.
Proposed solution: A wrapper for Curses (in port ncurses) which selects the right behaviour depending on CURSES_NEED_NCURSES.
There is a release lib "form" referenced in the debug build.
Reason: The debug variant from port ncurses has a "_g" debug postfix. The Curses find module doesn't know that.
Side note: Even the installed pc files don't know that.
Proposed solution: Get rid of the postfix.

To unblock the other changes in this PR, I will remove Curses from the test for now.

To fully follow recommended CMake package practices, I will also check <Pkg>_VAR before <PKG>_VAR.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

If you feel able to do so, please consider adding a "license" field to the following files:

ports/freetype/vcpkg.json
ports/icu/vcpkg.json
scripts/test_ports/cmake-user/vcpkg.json

Valid values for the license field can be found in the documentation

dg0yt · 2022-03-29T06:30:24Z

2. Proposed solution: Get rid of the postfix.

Note to self: The postfix is from CF_LIB_TYPE in aclocal.m4.

strega-nil-ms · 2022-03-29T17:24:32Z

This seems to have randomly fixed qtdeclarative, which results in qtinterfaceframework now running (and failing)...

BillyONeal · 2022-03-29T17:35:07Z

This seems to have randomly fixed qtdeclarative, which results in qtinterfaceframework now running (and failing)...

If the PR just happened to build less, given the diagnosis in #23755 that makes sense that it would 'randomly fix'.

dg0yt · 2022-03-29T18:57:55Z

This PR didn't fix that. The qtdeclarative regression depends on the number of installed ports.

strega-nil-ms · 2022-03-30T00:18:17Z

/azp run

azure-pipelines · 2022-03-30T00:18:26Z

Azure Pipelines successfully started running 1 pipeline(s).

dg0yt · 2022-03-30T04:15:15Z

REGRESSIONS:
PASSING, REMOVE FROM FAIL LIST: ompl:x64-osx (/Users/vagrant/Data/work/1/s/scripts/azure-pipelines/../ci.baseline.txt).
REGRESSION: omplapp:x64-osx failed with BUILD_FAILED. If expected, add omplapp:x64-osx=fail to /Users/vagrant/Data/work/1/s/scripts/azure-pipelines/../ci.baseline.txt.

Hm, this used to be tested in daily CI only, not in PR CI.

dg0yt · 2022-03-30T05:09:45Z

CI bug: #23862

strega-nil-ms · 2022-03-31T16:05:49Z

Since x64-osx is only failing due to #23862, I'm good with merging as is; I'll ask one other person to confirm then merge.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

If you feel able to do so, please consider adding a "license" field to the following files:

ports/freetype/vcpkg.json
ports/icu/vcpkg.json
scripts/test_ports/cmake-user/vcpkg.json

Valid values for the license field can be found in the documentation

BillyONeal · 2022-04-01T02:31:30Z

Since x64-osx is only failing due to #23862, I'm good with merging as is; I'll ask one other person to confirm then merge.

Ah, sorry I missed this before pushing a master merge. :/ At least we'll see that things are fixed :)

dg0yt · 2022-04-01T18:58:14Z

All green now.

strega-nil-ms · 2022-04-01T20:30:52Z

Thanks @dg0yt!

Verify library location for debug vs. release

d2c69da

github-actions bot approved these changes Nov 24, 2021

View reviewed changes

JonLiu1993 assigned JackBoosY Nov 24, 2021

JackBoosY 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 Nov 24, 2021

github-actions bot approved these changes Nov 29, 2021

View reviewed changes

JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label Nov 29, 2021

dg0yt changed the title ~~[cmake-user] Verify library location for debug vs. release~~ [cmake-user] Verify library location for debug vs. release, fix ports Nov 29, 2021

dg0yt marked this pull request as ready for review November 29, 2021 09:35

dg0yt commented Nov 29, 2021

View reviewed changes

ports/bzip2/portfile.cmake Outdated Show resolved Hide resolved

dg0yt force-pushed the cmake-user branch from fdf1cf1 to 43e489c Compare November 29, 2021 20:55

github-actions bot approved these changes Nov 29, 2021

View reviewed changes

JackBoosY suggested changes Nov 30, 2021

View reviewed changes

ports/freetype/vcpkg-cmake-wrapper.cmake Show resolved Hide resolved

JackBoosY added requires:author-response requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Nov 30, 2021

github-actions bot approved these changes Nov 30, 2021

View reviewed changes

JackBoosY removed the requires:author-response label Nov 30, 2021

christophe-calmejane mentioned this pull request Nov 30, 2021

OpenSSL x86-windows-static not working with find_package(OpenSSL REQUIRED) #12001

Closed

dg0yt mentioned this pull request Dec 1, 2021

[vcpkg] Add special functions for wrappers #21783

Closed

dg0yt marked this pull request as draft January 4, 2022 06:40

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

dg0yt and others added 5 commits February 28, 2022 09:16

Use existing variables for wrapper

ec6094f

Always install freetype wrapper

1fa9d01

Quote @expressions

34e9881

Set policies for freetype wrapper (microsoft#19459)

97b0194

Test JPEG

488e69e

JackBoosY approved these changes Mar 28, 2022

View reviewed changes

JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Mar 28, 2022

strega-nil-ms suggested changes Mar 28, 2022

View reviewed changes

ports/icu/vcpkg-cmake-wrapper.cmake Show resolved Hide resolved

scripts/test_ports/cmake-user/project/CMakeLists.txt Outdated Show resolved Hide resolved

scripts/test_ports/cmake-user/project/CMakeLists.txt Outdated Show resolved Hide resolved

Nicole CRs

221f1e3

github-actions bot approved these changes Mar 28, 2022

View reviewed changes

dg0yt added 2 commits March 29, 2022 08:10

Disable Curses test

9ddab8c

Prefer <Pkg>_VAR over <PKG>_VAR

fd927b8

github-actions bot approved these changes Mar 29, 2022

View reviewed changes

dan-shaw added the depends:vcpkg-team label Mar 30, 2022

strega-nil-ms self-requested a review March 31, 2022 19:04

strega-nil-ms approved these changes Mar 31, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into HEAD

ace63df

github-actions bot approved these changes Apr 1, 2022

View reviewed changes

strega-nil-ms merged commit cb91b41 into microsoft:master Apr 1, 2022

dg0yt deleted the cmake-user branch April 4, 2022 07:33

dg0yt mentioned this pull request Apr 10, 2022

[cmake-user] Fix libraries check #24070

Merged

Conversation

dg0yt commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Uh oh!

dg0yt commented Nov 29, 2021

Uh oh!

Uh oh!

JackBoosY commented Nov 30, 2021

Uh oh!

dg0yt commented Dec 20, 2021

Uh oh!

JackBoosY commented Dec 21, 2021

Uh oh!

dg0yt commented Jan 2, 2022

Uh oh!

strega-nil-ms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

dg0yt commented Mar 29, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

dg0yt commented Mar 29, 2022

Uh oh!

strega-nil-ms commented Mar 29, 2022

Uh oh!

BillyONeal commented Mar 29, 2022

Uh oh!

dg0yt commented Mar 29, 2022

Uh oh!

strega-nil-ms commented Mar 30, 2022

Uh oh!

azure-pipelines bot commented Mar 30, 2022

Uh oh!

dg0yt commented Mar 30, 2022

Uh oh!

dg0yt commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil-ms commented Mar 31, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Apr 1, 2022

Uh oh!

dg0yt commented Apr 1, 2022

Uh oh!

strega-nil-ms commented Apr 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dg0yt commented Nov 24, 2021 •

edited

Loading

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

dg0yt commented Mar 30, 2022 •

edited

Loading