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

[fastrtps] Fixes missing fast-discovery-serverd-1.0.1.exe #39482

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Cheney-W
Copy link
Contributor

Fixes #39375

Usage test passed with x64-windows

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@Cheney-W Cheney-W added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Jun 24, 2024
@Cheney-W Cheney-W marked this pull request as ready for review June 25, 2024 02:14
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Jun 25, 2024
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.

I think the issue wasn't properly understood.

This port doesn't use the regular maintainer function vcpkg_copy_tools which might be the entire reason for the reported issue: vcpkg_cmake_config_fixup is expecting a different tool location.

But there is also "debug" aspect: @BillyONeal @ras0219-msft After the dbus-daemon discussion, can you please double check and advise for this PR:

  • Port to vcpkg_copy_tools?
  • Keep debug executables?
  • Put the debug executable to /debug/tools/<port>?

I do understand that vcpkg_copy_tools becomes clumsy to use when there are scripts in addition to binaries. But improving things should be left to the community alone.

@BillyONeal BillyONeal added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed info:reviewed Pull Request changes follow basic guidelines labels Jun 27, 2024
@BillyONeal
Copy link
Member

But there is also "debug" aspect: @BillyONeal @ras0219-msft After the dbus-daemon discussion, can you please double check and advise for this PR:

Tagged to discuss this

@BillyONeal
Copy link
Member

@Cheney-W It seems like the bug is that the CMake targets are referring to the debug tool rather than referring to the release tool. Alternately, the right fix might be to remove the references from the targets entirely.

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 28, 2024
@BillyONeal BillyONeal marked this pull request as draft June 28, 2024 21:37
@dg0yt
Copy link
Contributor

dg0yt commented Jun 30, 2024

The debug tool seems to have a slightly different filename. That's why the default behavior of vcpkg_cmake_config_fixup isn't sufficient. (It maps everything to /tools/<port> but it doesn't adjust filenames.)

Whether the port needs to keep the debug tool, and where, are separate questions.

@Cheney-W
Copy link
Contributor Author

Cheney-W commented Jul 1, 2024

If we want to remove the reference to fast-discovery-server-targets, we need to delete this content:
https://github.com/eProsima/Fast-DDS/blob/d3ca40cbce231c4bdb4fc88a4b3911afaaae4f6f/cmake/packaging/Config.cmake.in#L92
@INCLUDE_FASTDDS_TARGETS@
However, this seems to go against the intention of the upstream.

From below code:
https://github.com/eProsima/Fast-DDS/blob/d3ca40cbce231c4bdb4fc88a4b3911afaaae4f6f/tools/fds/CMakeLists.txt#L72
set_target_properties(${PROJECT_NAME} PROPERTIES DEBUG_POSTFIX d-${PROJECT_VERSION})
it appears that the upstream intentionally differentiates the tool names generated in debug and release modes. Therefore, my original plan was just to place the tools generated in debug mode under tool\debug\bin.

@Cheney-W Cheney-W marked this pull request as ready for review July 18, 2024 02:44
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Jul 24, 2024
@zhangzhen5729
Copy link

I reinstalled vcpkg and executed git pull and vcpkg update to install fastrtps2.14.0, but the issue still persists and has not been fixed

@dg0yt
Copy link
Contributor

dg0yt commented Jul 25, 2024

I still think this port needs better restructuring, for canonical fixup. Not another variant of debug tool installation pattern unless applying for the vcpkg tool installation layout hall of shame.
And there is also #39954.

@Cheney-W
Copy link
Contributor Author

If we remove the d suffix from below codes:
https://github.com/eProsima/Fast-DDS/blob/d3ca40cbce231c4bdb4fc88a4b3911afaaae4f6f/tools/fds/CMakeLists.txt#L72
set_target_properties(${PROJECT_NAME} PROPERTIES DEBUG_POSTFIX d-${PROJECT_VERSION})
allowing this port to generate tool files with the same name in both debug and release builds, do you think this approach would work?
This method is actually a common practice used by most ports generating tools in vcpkg, but it does go against the original upstream design here. I'm not sure if this change is appropriate, but it is certainly the simplest solution.

@data-queue data-queue marked this pull request as draft July 27, 2024 08:23
@BillyONeal BillyONeal added depends:tool-ports Needs tool ports to be a better understood area before proceeding and removed info:reviewed Pull Request changes follow basic guidelines labels Jul 30, 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 depends:tool-ports Needs tool ports to be a better understood area before proceeding info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fastrtps] Missing fast-discovery-serverd-1.0.1.exe during construction
5 participants