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

[ignition-cmake2/ignition-modularscripts/ignition-common3] Add options, support pkgconfig and fix usage for common3-graph #25021

Merged

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Jun 1, 2022

Fix usage when using find_package(ignition-common3-graphics CONFIG REQUIRED):

CMake Error at /home/work/vcpkg/scripts/buildsystems/vcpkg.cmake:573 (_add_executable):
  Target "sample" links to target "GTS::GTS" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:45 (add_executable)


CMake Error at /home/work/vcpkg/scripts/buildsystems/vcpkg.cmake:573 (_add_executable):
  Target "sample" links to target "FreeImage::FreeImage" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:45 (add_executable)


CMake Error at /home/work/vcpkg/scripts/buildsystems/vcpkg.cmake:573 (_add_executable):
  Target "sample" links to target "TINYXML2::TINYXML2" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:45 (add_executable)

Related: #24969

Already tested this usage in x64-linux.

@JackBoosY JackBoosY 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 1, 2022
@JackBoosY JackBoosY requested a review from LilyWangLL June 1, 2022 07:07
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Jun 1, 2022
@talregev
Copy link
Contributor

talregev commented Jun 1, 2022

Thank you for the fix

@ras0219-msft ras0219-msft added the depends:different-pr This PR or Issue depends on a PR which has been filed label Jun 1, 2022
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

First, we should wait for #25034 to merge which adds a standardized method for consuming GTS (pkg-config).

Then,

  1. FindGTS.cmake should be replaced/implemented via the pkgconfig mechanism above
  2. ign_find_package should probably be patched to make PRIVATE_FOR not suppress find_dependency
  3. We should not need a vcpkg-cmake-wrapper here. Any changes needed should be written into the -config.cmake created by the port.

@JackBoosY
Copy link
Contributor Author

@ras0219-msft The official doc said:

# [PRIVATE_FOR]: Optional. If provided, the list that follows it must indicate
#                which library components depend on this package privately (i.e.
#                the package should not be included in its list of interface
#                libraries). This is only relevant for components that follow
#                the REQUIRED_BY command. Note that the PRIVATE argument does
#                not apply to components specified by REQUIRED_BY. This argument
#                MUST be given for components whose private dependencies have
#                been specified with REQUIRED_BY.

This parameter only affects cmake-config.cmake and pkgconfig.pc:
https://github.com/gazebosim/gz-cmake/blob/5adbd46a4e8a96f2bc67538bcca2406bb464ace3/cmake/IgnUtils.cmake#L325-L327

      foreach(component ${ign_find_package_PRIVATE_FOR})
        set(${component}_${PACKAGE_NAME}_PRIVATE true)
      endforeach()

https://github.com/gazebosim/gz-cmake/blob/5adbd46a4e8a96f2bc67538bcca2406bb464ace3/cmake/IgnUtils.cmake#L332-L336

      foreach(component ${ign_find_package_REQUIRED_BY})
        if(NOT ${component}_${PACKAGE_NAME}_PRIVATE)
          ign_string_append(${component}_CMAKE_DEPENDENCIES "${${PACKAGE_NAME}_find_dependency}" DELIM "\n")
        endif()
      endforeach()

https://github.com/gazebosim/gz-cmake/blob/5adbd46a4e8a96f2bc67538bcca2406bb464ace3/cmake/IgnUtils.cmake#L433-L439

            if(${component}_${PACKAGE_NAME}_PRIVATE)
              # If this is a private library or module, use the _PRIVATE suffix
              set(${component}_${PACKAGE_NAME}_PKGCONFIG_TYPE ${component}_${${PACKAGE_NAME}_PKGCONFIG_TYPE}_PRIVATE)
            else()
              # Otherwise, use the plain type
              set(${component}_${PACKAGE_NAME}_PKGCONFIG_TYPE ${component}_${${PACKAGE_NAME}_PKGCONFIG_TYPE})
            endif()

https://github.com/gazebosim/gz-cmake/blob/5adbd46a4e8a96f2bc67538bcca2406bb464ace3/cmake/IgnUtils.cmake#L433-L443

            if(${component}_${PACKAGE_NAME}_PRIVATE)
              # If this is a private library or module, use the _PRIVATE suffix
              set(${component}_${PACKAGE_NAME}_PKGCONFIG_TYPE ${component}_${${PACKAGE_NAME}_PKGCONFIG_TYPE}_PRIVATE)
            else()
              # Otherwise, use the plain type
              set(${component}_${PACKAGE_NAME}_PKGCONFIG_TYPE ${component}_${${PACKAGE_NAME}_PKGCONFIG_TYPE})
            endif()

            # Append the entry as a string onto the component-specific variable
            # for whichever required type we selected
            ign_string_append(${${component}_${PACKAGE_NAME}_PKGCONFIG_TYPE} ${${PACKAGE_NAME}_PKGCONFIG_ENTRY})

https://github.com/gazebosim/gz-cmake/blob/ign-cmake2/cmake/ignition-component-config.cmake.in#L80

@component_cmake_dependencies@

In fact, the configuration of cmake should be the same as that of pkgconfig. So I think "make PRIVATE_FOR not suppress find_dependency" is consistent with the modification (current modification) of directly removing PRIVATE_FOR from these ports that should be exported.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 2, 2022

In fact, the configuration of cmake should be the same as that of pkgconfig. So I think "make PRIVATE_FOR not suppress find_dependency" is consistent with the modification (current modification) of directly removing PRIVATE_FOR from these ports that should be exported.

I cannot test it now but I disagree. I expect that your current modification changes the link type from PRIVATE to PUBLIC. This would be notable in the generated config by losing the $<LINK_ONLY:...> wrapping in INTERFACE_LINK_LIBRARIES properties, having side effects on consumers (e.g. adding definitions and include paths).
That's the whole point: You need a find_dependency for every library (which is not header-only), to satisfy static linkage.

@LilyWangLL LilyWangLL removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Jun 13, 2022
@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Jun 15, 2022
@JackBoosY JackBoosY changed the title [ignition-common3] Fix usage for graph [ignition-cmake2/ignition-modularscripts/ignition-common3] Add options, support pkgconfig and fix usage for common3-graph Jun 15, 2022
@JackBoosY
Copy link
Contributor Author

Already tested usage on x64-linux.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jun 15, 2022
@JackBoosY JackBoosY requested a review from ras0219-msft June 15, 2022 09:22
@dan-shaw dan-shaw merged commit 91393fa into microsoft:master Jun 17, 2022
@JackBoosY JackBoosY deleted the dev/jack/fix-ignition-comm3-graph-usage branch June 17, 2022 05:57
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:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants