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

[cmake-user] Verify library location for debug vs. release, fix ports #21641

Merged
merged 20 commits into from
Apr 1, 2022

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Nov 24, 2021

  • 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

@JackBoosY 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
@JackBoosY 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 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 dg0yt marked this pull request as ready for review November 29, 2021 09:35
ports/bzip2/portfile.cmake Outdated Show resolved Hide resolved
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 29, 2021

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

@JackBoosY 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
@JackBoosY
Copy link
Contributor

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

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 20, 2021

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
Copy link
Contributor

Can you please resolve the file conflicts?

Thanks.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 2, 2022

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

@dg0yt dg0yt marked this pull request as draft January 4, 2022 06:40
@JackBoosY 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
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Mar 28, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

minor changes requested

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
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.

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/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
Copy link
Contributor Author

dg0yt commented Mar 29, 2022

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:

  1. 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.
  2. 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.

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.

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/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
Copy link
Contributor Author

dg0yt commented Mar 29, 2022

2. Proposed solution: Get rid of the postfix.

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

@strega-nil-ms
Copy link
Contributor

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

@BillyONeal
Copy link
Member

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
Copy link
Contributor Author

dg0yt commented Mar 29, 2022

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

@strega-nil-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 30, 2022

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
Copy link
Contributor Author

dg0yt commented Mar 30, 2022

CI bug: #23862

@dan-shaw dan-shaw added the depends:vcpkg-team This PR depends on the vcpkg team to solve something label Mar 30, 2022
@strega-nil-ms
Copy link
Contributor

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.

@strega-nil-ms strega-nil-ms self-requested a review March 31, 2022 19:04
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.

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/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
Copy link
Member

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
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

All green now.

@strega-nil-ms strega-nil-ms merged commit cb91b41 into microsoft:master Apr 1, 2022
@strega-nil-ms
Copy link
Contributor

Thanks @dg0yt!

@dg0yt dg0yt deleted the cmake-user branch April 4, 2022 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed depends:vcpkg-team This PR depends on the vcpkg team to solve something info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants