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-scripts] Fix rpath fixup; add test #37964

Merged
merged 9 commits into from
Apr 23, 2024

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Apr 4, 2024

Fix fixup for debug tools, #37736 (comment).
Fix fixup for paths with regex chars, #37984.

@dg0yt dg0yt changed the title Fix rpath fixup for debug tools; add test Fix rpath fixup; add test Apr 5, 2024
@JonLiu1993 JonLiu1993 added category:port-bug The issue is with a library, which is something the port should already support category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly and removed category:port-bug The issue is with a library, which is something the port should already support labels Apr 7, 2024
@JonLiu1993 JonLiu1993 changed the title Fix rpath fixup; add test [vcpkg-scripts] Fix rpath fixup; add test Apr 7, 2024
JonLiu1993
JonLiu1993 previously approved these changes Apr 7, 2024
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Apr 7, 2024
@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 9, 2024
@JavierMatosD
Copy link
Contributor

@ras0219, @data-queue, @vicroms, @BillyONeal, and I agree that fixing regex handling is absolutely good. However, expanding the definition of where debug tools should go depends on the specific use cases and we don't know of any generic use cases at this time.

That said, the DBUS situation specifically is unusual and we do believe we need to deploy the debug Daemon exe.

@JavierMatosD JavierMatosD marked this pull request as draft April 12, 2024 13:55
@JavierMatosD JavierMatosD removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 12, 2024
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 13, 2024

expanding the definition of where debug tools should go depends on the specific use cases and we don't know of any generic use cases at this time.

Sorry, but I cannot follow.
Do you want to say that tools/<port>/debug and manual-tools/<port>/debug may contain something else than debug tools?
tools/<port>/debug/bin is populated by vcpkg_configure/build/install_make, Qt5 and tcl, so there is a valid defect if the rpaths aren't fixed (EDIT:) but this is already handled by the current script.

For numbers, see #37736 (comment). Of course, this includes foo-config scripts.
For limiting the ongoing expansion, please resolve #17607.

@dg0yt dg0yt marked this pull request as ready for review April 13, 2024 04:35
@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 22, 2024
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

THANK YOU for the tests.

@JavierMatosD JavierMatosD merged commit a59ba45 into microsoft:master Apr 23, 2024
16 checks passed
@dg0yt dg0yt deleted the fixup-rpath branch April 23, 2024 19:09
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 23, 2024

Nice, can we move the dbus debug tools to a proper location now?

@BillyONeal
Copy link
Member

Nice, can we move the dbus debug tools to a proper location now?

My understanding is that the dbus situation is different because it's about how the library finds the exe but I didn't pay super close attention to the details there; I poked @data-queue and @ras0219-msft who did

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 24, 2024

it's about how the library finds the exe

I don't think I ever saw a mention of this twist. It was always about the exes finding their libs. And a little bit about cmake prefix and find stuff. Nothing specific about dbus, except the reason why the debug tools would be needed.

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Apr 24, 2024

Nothing specific about dbus, except the reason why the debug tools would be needed.

The specific weird thing about dbus is that the executable is launched by the library, which means it's like a configuration-dependent "cacert.pem" thing that needs to be deployed with the final user's application. This means that users of the debug version of libdbus actually do need a debug variant of the executable dbus-daemon.

It is a non-goal (but acceptable) that the debug variant of dbus-daemon is executable from the installed directory; it eventually needs to be copied by the user into their overall product and probably rpath-adjusted to work in that environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants