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

Update installation-tree-layout.md - update tools/<port> instructions #292

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

tsondergaard
Copy link
Contributor

Make it clear that in cases where the executables will be used at runtime then both release and debug versions should be included. This is important in general, but for practical reasons especially with dynamic linking. Application developers should be able to copy the right version of the tool to the bin/ folder in the install prefix and have it work with both release and debug builds.

Make it clear that in cases where the executables will be used at runtime then both release and debug versions should be included. This is important in general, but for practical reasons especially with dynamic linking. Application developers should be able to copy the right version of the tool to the bin/ folder in the install prefix and have it work with both release and debug builds.
Copy link

@tsondergaard : Thanks for your contribution! The author(s) have been notified to review your proposed change.

Copy link

@tsondergaard : Thanks for your contribution! The author(s) have been notified to review your proposed change.

Copy link
Contributor

Learn Build status updates of commit 003d6d4:

✅ Validation status: passed

File Status Preview URL Details
vcpkg/reference/installation-tree-layout.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

The vcpkg team has never been able to give satisfying answers to installation layout questions for tools. Cf. microsoft/vcpkg#17607.

Comment on lines +154 to +155
> Both release and debug executables should be provided when the executables are intended
> for runtime use.
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt that it is so simple. Many packages I deal with explode in size when installing debug executables for static linkage, e.g. gdal. (And some ports are already not cacheable in vcpkg CI with release executables only, in particular llvm.) Basically, when I want to debug executables, I would probably not use the stock port recipe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is not just to allow debugging of these executables. With dynamic linking these executables will typically have shared library dependencies and that means if you want to be able to install your project to CMAKE_INSTALL_PREFIX then everything you install should depend only on the debug version of libraries or the release version of libraries - not a mix. This is probably easier to describe with an example. Let's say my project depends on dbus and I make a debug build of my project and I want to install my project to the CMAKE_INSTALL_PREFIX with cmake --build --target install. Everything in this case would include dbus-daemon and dbus-launch from the dbus vcpkg port, but those two executables are only available in debug versions. When I install these dynamic executables they need to include their dependencies and since they are always from the release build of dbus that means I need to install both the debug and the release versions of libdbus-1.so in the same install prefix. That's messed up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the dbus dilemma, but the proposed wording introduces other dilemmas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What dilemmas are that? Very few packages need to deliver executables and the existing sentence that I am tagging this onto are already softly worded:

vcpkg is first and foremost a C++ library dependency manager. Port authors should
be deliberate when deciding to include tools in the installation output. For example:
consider installing only a release executable when the debug tool is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

"intended for runtime use" also matches executables like sqlite3, gdal, or unzip. But we don't really want to install them in general, even if the user enabled the controlling features. That is "the debug tool is not needed."

The dbus situation mighe be better describted with:

Suggested change
> Both release and debug executables should be provided when the executables are intended
> for runtime use.
> Both release and debug executables should be provided when the executables are required
> to be started alongside user executables with a shared set of runtime libraries.
> Example are `dbus-daemon` and `dbus-launch`.

Disclaimer: I'm a community member, and not a native speaker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also not a native speaker. And I see your point, and like your suggested alternative. I will await further feedback before committing your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

@ras0219-msft updated our stance on this matter here: microsoft/vcpkg#17607 (comment)

We ruled that debug executables are acceptable in some special cases but I feel the disclaimer in our documentation should be more emphatic on what we consider to be acceptable special cases.

We intend to update the maintainer policy to explicitly mention this topic. So, you could wait until the policy is updated and then link to that from this article.

@vicroms vicroms merged commit 86737d5 into microsoft:main Jul 15, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants