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-tool] vcpkg install <pkg> gives wrong usage information #20190

Open
dg0yt opened this issue Sep 16, 2021 · 45 comments
Open

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

dg0yt opened this issue Sep 16, 2021 · 45 comments
Assignees
Labels
category:port-bug The issue is with a library, which is something the port should already support requires:discussion

Comments

@dg0yt
Copy link
Contributor

dg0yt commented Sep 16, 2021

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 #14994 (comment)

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 16, 2021

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

dg0yt commented Sep 16, 2021

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

@NancyLi1013 NancyLi1013 added the category:port-bug The issue is with a library, which is something the port should already support label Sep 17, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 7, 2021

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

@dg0yt dg0yt changed the title [vcpkg-tool] vcpkg install libiconv gives wrong usage information [vcpkg-tool] vcpkg install <pkg> gives wrong usage information Oct 11, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 19, 2021

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

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 21, 2021

CC @ras0219-msft @strega-nil-ms

@strega-nil-ms
Copy link
Contributor

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

@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 8, 2022

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

dg0yt commented Jan 20, 2022

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

@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 29, 2022

Added qt5-base.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 31, 2022

Added azure-iot-sdk-c

@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 16, 2022

Added qhull.

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

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 5, 2022

Added libpng.

Ping @vicroms @strega-nil-ms.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 12, 2022

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

dg0yt commented Jun 2, 2022

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

dg0yt commented Sep 19, 2022

The tool also creates invalid suggestions by copying things literally, e.g.
target_link_libraries(main PRIVATE <name> alpaka alpaka::alpaka) (#26812 (comment)), or
target_link_libraries(main PRIVATE # foobar), or
target_link_libraries(main PRIVATE " cmrc-base cmrc::base) (#27109 (comment))

@FabienPean
Copy link
Contributor

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:

  • From this thread, I understand that the current recommended way is to add a usage file in the port folder ?
  • If so, is there any specific template or format to follow for getting quick approval of the PR ?

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

dg0yt commented Nov 23, 2022

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

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

dg0yt commented Jan 18, 2023

@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,

  • most targets are for a particular purpose. They shouldn't be used when they aren't needed.
  • some targets are for shared code.They shouldn't be used directly, but are included as transitive usage requirements.

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

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

dg0yt commented Jan 20, 2023

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

dg0yt commented Jan 25, 2023

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

dg0yt commented Jan 25, 2023

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

Copy link

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.

@github-actions github-actions bot added the Stale label Sep 17, 2024
@Cheney-W
Copy link
Contributor

Stay active

@jimwang118 jimwang118 removed the Stale label Sep 18, 2024
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 requires:discussion
Projects
None yet
Development

No branches or pull requests