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

[msquic] Do not rename binaries #40947

Merged
merged 1 commit into from
Sep 13, 2024
Merged

[msquic] Do not rename binaries #40947

merged 1 commit into from
Sep 13, 2024

Conversation

@BillyONeal
Copy link
Member

How is this renaming the binary? It's changing the name in the build, not renaming it afterwards.

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

dg0yt commented Sep 13, 2024

TLDR: The HOW is okay, but the WHAT is already wrong.

How is this renaming the binary?

It appends a d to debug libraries. But the first part of the guideline clearly says:

"if the upstream library has the same name in release and debug, we should not introduce a new name."

This is about not breaking upstream's choice aand downstreams' expectations as they might be encoded in find_library or deployment scripts.

It's changing the name in the build, not renaming it afterwards.

This is another part of the guideline. If binaries are renamed (which is allowed/encouraged by "Static and shared variants often should be renamed to a common scheme"), then it must be done in this way.

@redbaron
Copy link
Contributor

in practice having a same DLLs with same name, but incompatible in the runtime and even link time leads to some very odd behaviour. For instance before 40938 nothing was broken at build time, CMake was happily linking /MDd binary with /MD msquic.dll and install(IMPORTED_RUNTIME_ARTIFACTS msquic DESTINATION ${CMAKE_INSTALL_LIBDIR}) was installing wrong copy of msquic.dll when packaging build result.

adding d suffix is pretty common and it is done not via rename simple rename after the fact, which would be bad indeed. msquic is CMake based so new name is picked up by msquic-config.cmake correctly

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 13, 2024

No new information. Not different from other ports. vcpkg has different roots to separate debug from release. Upstream determines the names, and the ports shall not change it.

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 13, 2024

install(IMPORTED_RUNTIME_ARTIFACTS msquic DESTINATION ${CMAKE_INSTALL_LIBDIR}) was installing wrong copy of msquic.dll when packaging build result.

Who calls that? Who initializes the search paths for runtime artifacts?

@redbaron
Copy link
Contributor

Who calls that? Who initializes the search paths for runtime artifacts?

Consumer of msquic, my project which uses msquic for deps. that is what prompted #40938 , because debug build was picking wrong DLL when using Ninja-Multi generator.

nothing fancy otherwise:

find_package(msquic CONFIG REQUIRED)
target_link_libraries(mylib PRIVATE msquic)
install(IMPORTED_RUNTIME_ARTIFACTS msquic DESTINATION ${CMAKE_INSTALL_LIBDIR})

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 13, 2024

Multi-config, the incomplete story...
But I would take IMPORTED_RUNTIME_ARTIFACTS as another indication that the original problem should have been solved by adding the missing vcpkg_cmake_config_fixup alone, properly pulling the debug artifacts into the imported target.

@redbaron
Copy link
Contributor

redbaron commented Sep 13, 2024

problem should have been solved by adding the missing vcpkg_cmake_config_fixup alone

CMAKE_DEBUG_POSTFIX is just a safety net for silent breakages. Coming from linux, it is wild to me that incompatible DLLs can intentionally be named the same on filesystem.

If there is a guidelines not to rename from upstream, I guess I should take this change upstream then

@BillyONeal BillyONeal merged commit afdfa27 into microsoft:master Sep 13, 2024
16 checks passed
@BillyONeal
Copy link
Member

"if the upstream library has the same name in release and debug, we should not introduce a new name."

I agree, I was thinking of a different bit about renaming DLLs post build.

@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Sep 14, 2024
@dg0yt dg0yt deleted the msquic branch September 17, 2024 16:22
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 info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants