search

microsoft / vcpkg

C++ Library Manager for Windows, Linux, and MacOS
MIT License
22.88k stars 6.31k forks source link

[vcpkg-tool] `vcpkg install <pkg>` gives wrong usage information #20190

Open dg0yt opened 3 years ago

dg0yt commented 3 years ago

Bad User Experience

The auto-generated usage information is wrong for many non-trivial ports.

Earlier reports

Known problem ports

Patched by usage file

Other resolved

Misguided patching in vcpkg

Promoting Bad Practice

All targets are listed in the same target_link_libraries line. But normally, canonical usage is to use only a single desired target and let it pull in its transitive usage requirements.

Some package do export targets not as public interface, but as an implementation detail hidden behind interface variables. By automatic reporting of targes, vcpkg distracts user from applying canonical usage, contributors from researching, documenting and testing canonical usage, and team members to base there support answers on canonical usage.

Example: User experience for libiconv

but when i run the command vcpkg install libiconv, it always tell me

ubuntu@o-01:~/vcpkg$ vcpkg install libiconv
Computing installation plan...
The following packages are already installed:
    libiconv[core]:x64-linux -> 1.16#11
Package libiconv:x64-linux is already installed
Restored 0 packages from /home/ubuntu/.cache/vcpkg/archives in 1.052 us. Use --debug to see more details.

Total elapsed time: 37.06 us

The package libiconv provides CMake targets:

    find_package(iconv CONFIG REQUIRED)
    target_link_libraries(main PRIVATE Iconv::Charset)

please fix the issue.

Originally posted by @ssrlive in https://github.com/microsoft/vcpkg/issues/14994#issuecomment-920596707

dg0yt commented 3 years ago

The CONFIG keyword and the package name (actually Iconv) and the target name (actually Iconv::Iconv) are wrong. It should be:

find_package(Iconv REQUIRED)
target_link_libraries(main PRIVATE Iconv::Iconv)

Generally, given a share/<pkg>/vcpkg-cmake-wrapper.cmake, the tool heuristics would need to look for a Find<pkg>.cmake (case insensitively), and suggest find_package(<pkg-with-rectified-case> REQUIRED) if such a module is found.

dg0yt commented 3 years ago

In addition, Iconv::Charset is an internal helper of the wrapper, not a public feature.

dg0yt commented 2 years ago

I see this as a general vcpkg bug now. Usage information must be reviewed and verified, or omitted. Maybe something for "discussion".

dg0yt commented 2 years ago

Is there sufficient evidence now to request a response from the vcpkg team?

dg0yt commented 2 years ago

CC @ras0219-msft @strega-nil-ms

strega-nil-ms commented 2 years ago

This is fair enough; I'll bring it up in the new year at the PR meeting.

dg0yt commented 2 years ago 
The following packages will be built and installed:
    drogon[core,mysql,orm]:x64-linux -> 1.7.4
....
The package drogon provides CMake targets:

    find_package(Drogon CONFIG REQUIRED)
    # Note: 6 target(s) were omitted.
    target_link_libraries(main PRIVATE pg_lib UUID_lib coz::coz MySQL_lib)

The Drogon config clearly provides target Drogon::Drogon. None of the other targets should be linked manually. They seem to come from Find modules installed but only conditionally used via find_dependency(...). Maybe a hint how to improve the heuristic.

dg0yt commented 2 years ago

This is a friendly reminder. Actually I'm increasingly unhappy with how this vcpkg feature promotes bad practice.

dg0yt commented 2 years ago

Added qt5-base.

dg0yt commented 2 years ago

Added azure-iot-sdk-c

dg0yt commented 2 years ago

Added qhull.

Do I need to make a PR which disables the tool feature?

dg0yt commented 2 years ago

Added libpng.

Ping @vicroms @strega-nil-ms.

dg0yt commented 2 years ago

Today's highlight from my draft gdal-cmake build, after continuing work on exported CMake config.

The package gdal provides CMake targets:

    find_package(3.12 CONFIG REQUIRED)
    # Note: 6 target(s) were omitted.
    target_link_libraries(main PRIVATE PNG::PNG JPEG::JPEG Boost::boost EXPAT::EXPAT)

    find_package(3.13 CONFIG REQUIRED)
    target_link_libraries(main PRIVATE XercesC::XercesC)

    find_package(3.14 CONFIG REQUIRED)
    target_link_libraries(main PRIVATE CURL::libcurl)

    find_package(3.16 CONFIG REQUIRED)
    target_link_libraries(main PRIVATE PostgreSQL::PostgreSQL)

    find_package(GDAL CONFIG REQUIRED)
    target_link_libraries(main PRIVATE GDAL::GDAL)

    find_package(packages CONFIG REQUIRED)
    # Note: 59 target(s) were omitted.
    target_link_libraries(main PRIVATE DAP::DAP ECW::ECW GIF::GIF IDB::IDB)

As already mentioned two months ago, it is wrong to blindly report all find modules which are used by a cmake config.

dg0yt commented 2 years ago

@vicroms @strega-nil-ms @ras0219-msft @BillyONeal Issue open for 6 months with no adequate reaction. I could contribute to a solution more actively, but I refuse to work on these grounds.

JackBoosY commented 2 years ago

We will discuss this in the next meeting. Sorry for late reply.

jvelilla commented 2 years ago

El mar., 15 mar. 2022 03:24, Kai Pastor @.***> escribió:

Today's highlight from my draft gdal-cmake build, after continuing work on exported CMake config.

The package gdal provides CMake targets:

find_package(3.12 CONFIG REQUIRED)
# Note: 6 target(s) were omitted.
target_link_libraries(main PRIVATE PNG::PNG JPEG::JPEG Boost::boost EXPAT::EXPAT)

find_package(3.13 CONFIG REQUIRED)
target_link_libraries(main PRIVATE XercesC::XercesC)

find_package(3.14 CONFIG REQUIRED)
target_link_libraries(main PRIVATE CURL::libcurl)

find_package(3.16 CONFIG REQUIRED)
target_link_libraries(main PRIVATE PostgreSQL::PostgreSQL)

find_package(GDAL CONFIG REQUIRED)
target_link_libraries(main PRIVATE GDAL::GDAL)

find_package(packages CONFIG REQUIRED)
# Note: 59 target(s) were omitted.
target_link_libraries(main PRIVATE DAP::DAP ECW::ECW GIF::GIF IDB::IDB)

As already mentioned two months ago https://github.com/microsoft/vcpkg/issues/20190#issuecomment-1008044822, it is wrong to blindly report all find modules which are used by a cmake config.

— Reply to this email directly, view it on GitHub https://github.com/microsoft/vcpkg/issues/20190#issuecomment-1067609076, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFW6W3AC4JEGEM7PWPVI3VAAULFANCNFSM5EEBGF5Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

BillyONeal commented 2 years ago

@vicroms @strega-nil-ms @ras0219-msft @BillyONeal Issue open for 6 months with no adequate reaction. I could contribute to a solution more actively, but I refuse to work on these grounds.

I think you're not getting a response here because the problem described is really a structural problem that's going to require massive investment / spec / testing / etc. to resolve, and while the experience can be better, this bug hasn't risen to the top of priorities to spend multiple developer weeks to land a solution.

@dan-shaw @ras0219 @strega-nil @vicroms and I just discussed this in person with potential things we could do being encouraging people to provide more usage files when we merge things into the repo, or we could provide entries in vcpkg.json to make it easier to generate better usage information that we could test. We have been hesitant to add that until we had the time/capability to do checking / enforcement that the resulting find_package calls would work.

On the other hand we aren't sure how to interpret your message from 3 days ago ( https://github.com/microsoft/vcpkg/issues/20190#issuecomment-1067609076 ); it seems like nothing about how we maintain the catalog could make you happy here and nothing short of removal of all generated usages. On the other hand that may be a specific bug we could improve, but since this is stapled into the overall long term structural issue we aren't sure how to interpret your message.

dg0yt commented 2 years ago

I'm not sure if it is better to have users open the same issues again and again. Collecting the issues was meant to help adjust priorities and to incrementally approach low hanging fruits.

dg0yt commented 2 years ago

Implement a strict review policy.

@BillyONeal Recent example for a port which is already listed here, with a PR just approved while facing unusable usage information: https://github.com/microsoft/vcpkg/pull/23502#issuecomment-1073655939

dg0yt commented 2 years ago

Added mailio.

dg0yt commented 2 years ago

Adding jsoncpp. From https://github.com/microsoft/vcpkg/issues/23839#issuecomment-1082709408.

The package jsoncpp provides CMake targets:

    find_package(jsoncpp CONFIG REQUIRED)
    target_link_libraries(main PRIVATE jsoncpp_object jsoncpp_static JsonCpp::JsonCpp)
JackBoosY commented 2 years ago

We really need a post check to check the usages.

dg0yt commented 2 years ago

We really need a post check to check the usages.

However, the check can't detect all pointless proposal. It won't detect overlinking.

dg0yt commented 2 years ago

Adding realsense2. Output:

   target_link_libraries(main PRIVATE realsense2::fw realsense2::usb realsense2::realsense2 realsense2::realsense-file)

Reasonable:

   target_link_libraries(main PRIVATE realsense2::realsense2)
   target_link_libraries(main PRIVATE realsense2::fw)

I'm tempted to call this the "realnonsense" bug from now.

dg0yt commented 2 years ago

Adding iir1 and openxr-loader: Config in wrong dir. This could be noticed by the post-build check...

dg0yt commented 2 years ago

A particular issue is printing directory names instead of module names. This is what leads to 


find_package(cmake ...) # share/<port>/cmake/....cmake, e.g. mailio
find_package(3.14 ...) # share/<port>/3.14/Find<Pkg>.cmake, e.g. gdal
JackBoosY commented 2 years ago

A particular issue is printing directory names instead of module names. This is what leads to

find_package(cmake ...) # share/<port>/cmake/....cmake, e.g. mailio
find_package(3.14 ...) # share/<port>/3.14/Find<Pkg>.cmake, e.g. gdal

Normally. it should be caused by the incorrect installation path.

dg0yt commented 2 years ago

Normally. it should be caused by the incorrect installation path.

For mailio, perhaps. But then vcpkg tool would need to report a validation error, not an unusable usage information. For GDAL, no. These are utility find modules, needed for dependencies of the exported config (which is in the correct directory).

Basically, vcpkg tool must ignore for usage what isn't found by our CMake toolchain. Our CMake toolchain finds config or wrappers only directly in share/<pkg>. This is the base for the proposal in https://github.com/microsoft/vcpkg/issues/20190#issuecomment-1072067545.

strega-nil-ms commented 2 years ago

@dg0yt, at our meeting, we discussed this, and these are our thoughts:

I'm not sure if it is better to have users open the same issues again and again. Collecting the issues was meant to help adjust priorities and to incrementally approach low hanging fruits.

  • Implement a strict review policy.

Short-term, this would be really difficult to do in a way that's not prohibitively expensive on our engineers' time. Long-term, this would be really nice, tho.

We could add specific requirements to the maintainer guidelines for new ports, to review the autogenerated usage and make sure the find_package call resolves.

  • Never list more than one target in target_link_libraries(main PRIVATE ...). (We can assume that targets properly carry transitive usage requirements unless upstream is broken.) Test cases: azure-iot-sdk-c, drogon, civetweb
  • Basically, if there is still more than one target, don't report any target. (Issue a mesage to remind maintainers/reviewers to add a usage file.)

(later bullet moved up) We would like to continue listing more than one target, but perhaps with a warning message about adding a usage file, and warn/fail in CI

  • Ignore all Find modules. (We can assume they are just helpers or backports). Test cases: civetweb (more candidates via CI file lists)

I think we try to do this already, but we definitely should do this more if we ever don't

  • Find module usage: If there is vcpkg-cmake-wrapper.cmake, look for a <Pkg>::<Pkg> target, <Pkg>_INCLUDE_DIRS and <Pkg>_LIBRARIES in the official Find module from CMake. Maybe even print a link to the find module documentation url. Don't take variables or targets from vcpkg-cmake-wrapper. Test case: libiconv / FindIconv.cmake.

Agree that this should be a usage file, but doing this automatically would be difficult since we don't have a mapping of modules <-> ports

  • Config file usage: If there is a <Pkg>Config.cmake or <pkg>-config.cmake in a suitable directory, find <basename>-release.cmake and pick targets from <basename>.cmake. Prioritize by name <pkg>::lib<pkg>, `::, or by link libraries relationships. Test cases: drogon, civetweb

We think this should be subsumed by "if we detect multiple targets, tell the user to add a usage file" - we don't want to be heuristicking like this.

  • (And then there is pkgconfig usage, even with targets now in CMake.)

It would be good to add this feature to vcpkg (with a similar check for "if we have multiple pc files, add a usage")

On the other hand, this would likely be quite expensive, and the vcpkg team is unlikely to be able to do this in the short-term (or even medium-term); especially making sure that the CI fails would be a ton of work (see @autoantwort's work on absolute paths as an example of this). However, a useful minor change we could take from this is, for example, making our heuristically generated usage information less authoritative; perhaps adding a # this is heuristically generated, and may not be correct to the output would be good.

dg0yt commented 2 years ago

However, a useful minor change we could take from this is, for example, making our heuristically generated usage information less authoritative; perhaps adding a # this is heuristically generated, and may not be correct to the output would be good.

This sounds like a good first step.

dg0yt commented 2 years ago

Recent additions: c-ares, sentry-native

dg0yt commented 2 years ago

Added shapelib:

shapelib provides CMake targets:
    # this is heuristically generated, and may not be correct
    find_package(shp CONFIG REQUIRED)
    target_link_libraries(main PRIVATE shp)
$ ls packages/shapelib_x64-linux/share/shp/
export_shp.cmake  export_shp-debug.cmake  export_shp-release.cmake
dg0yt commented 2 years ago

shapelib bonus fact:

22633 added a patch file to fix the config, but didn't add it to the portfile.

Not detected in review. Not detected in automated checks. And IMO the patch is invalid because it makes assumptions about the desired package name. It must be unofficial until fixed upstream.

dg0yt commented 2 years ago

LOL while working on #24720:

Elapsed time to handle osgearth:x64-linux: 16.23 min

Total elapsed time: 16.28 min

osgearth is header-only and can be used from CMake via:

    find_path(OSGEARTH_INCLUDE_DIRS "osgEarth/AGG.h")
    target_include_directories(main PRIVATE ${OSGEARTH_INCLUDE_DIRS})

(Libs went to /lib64 by error. Headers don't have a .h suffix except for the one listed.)

Where is the # this is heuristically generated, and may not be correct?

dg0yt commented 2 years ago

The tool also creates invalid suggestions by copying things literally, e.g. target_link_libraries(main PRIVATE <name> alpaka alpaka::alpaka) (https://github.com/microsoft/vcpkg/pull/26812#issuecomment-1250955731), or target_link_libraries(main PRIVATE # foobar), or target_link_libraries(main PRIVATE " cmrc-base cmrc::base) (https://github.com/microsoft/vcpkg/pull/27109#issue-1398429661)

FabienPean commented 1 year ago

I stumbled upon this problem recently with ports Simbody, Blas, Lapack, and header-only Boost libraries.

Just to clarify @strega-nil-ms since I couldn't find a guideline on the topic:

Listing all the targets provided by a port in the usage file would be best to me, unless there is an easy way to list the installed targets provided by the port. I remember getting quite a few times # note: X additional targets are not displayed. forcing me to dig the corresponding share/<lib> folder.

dg0yt commented 1 year ago
  • From this thread, I understand that the current recommended way is to add a usage file in the port folder ?

Yes.

  • If so, is there any specific template or format to follow for getting quick approval of the PR ?

Look at a similar port, or start with the tool's ouput. But use your brain, and upstream documentation.

I remember getting quite a few times # note: X additional targets are not displayed. forcing me to dig the corresponding share/ folder.

Quite often, there are only a few relevant targets, and the others are used implicitly.

JoseSanchez7 commented 1 year ago

Hello @dg0yt. In regard to the azure-iot-sdk-c, what exactly are you referring to? An explicit example in the code base would be helpful.

dg0yt commented 1 year ago

@JoseSanchez7 Without looking at the current state:

azure-iot-sdk-c (lists 4 out of 10 targets in one line, too much)

vcpkg detects many targets and print some 4 of them. This is not a good path to indicate usage. In general,

The proper fix is to add a usage file to the port, connecting targets to purposes. Simple example: ports/geos/usage. Or just the most common case, and a pointer to upstream documentation. (Unfortunately, many upstream are particular in this aspect of documentation.)

JoseSanchez7 commented 1 year ago

Hello @dg0yt, our team (Azure C SDK) spoke yesterday morning about your request regarding changes to how our cmake works with vcpkg and decided that it's not something we will be changing at this moment.

dg0yt commented 1 year ago

@JoseSanchez7 I didn't want to indicate a need for changes to "how your cmake works". I see a need to better document downstream CMake usage. The heuristics in vcpkg tool is very limited. It is unlikely that a random pick of 4 targets out of 10 is what the users needs.

dg0yt commented 1 year ago

@BillyONeal Not sure if it was case the before the last tool release, but now the heuristics picks misleading information (CONFIG, targets) from the wrappers:

tiff provides CMake targets:

    # this is heuristically generated, and may not be correct
    find_package(tiff CONFIG REQUIRED)
    target_link_libraries(main PRIVATE ZSTD::ZSTD)
dg0yt commented 1 year ago

@BillyONeal Ignore my last comment. I bootstrapped on the old version. The new version no longer outputs misleading information for tiff. (Now we should probably looka at my next proposal: Check if there is an official find module when we find wrapper.)

github-actions[bot] commented 1 week ago

This is an automated message. Per our repo policy, stale issues get closed if there has been no activity in the past 180 days. The issue will be automatically closed in 14 days. If you wish to keep this issue open, please add a new comment.

Cheney-W commented 6 days ago

Stay active