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.

Sign up for GitHub

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

Jump to bottom

[ignition-plugin1] Add new port 🤖 #11275

Merged
merged 4 commits into from
Jun 24, 2020
Merged

[ignition-plugin1] Add new port 🤖 #11275

merged 4 commits into from
Jun 24, 2020

Conversation

traversaro
Copy link
Contributor

  • What does your PR fix?

    • This PR adds a new port for the new major version of the ignition-plugin library.

  • Which triplets are supported/not supported?

    • All tested triplet should be supported

  • Does your PR follow the maintainer guide?

    • I hope.

As discussed in #7781, different major version of ignition robotics libraries (https://ignitionrobotics.org/) can be installed side by side, so each new major version is added as a new port.

In particular, ignition-plugin1 is a dependency of Ignition Robotics Citadel (that contains Ignition Gazebo 3, see https://www.openrobotics.org/blog/2019/12/11/ignition-citadel-released).

@traversaro traversaro changed the title [ignition-plugin1] Add new port [ignition-plugin1] Add new port 🤖 May 9, 2020
@NancyLi1013
Copy link
Contributor

/azp run

1 similar comment
@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

@strega-nil
Could you please help handle the failure on osx pipeline?
##[error]Git checkout failed with exit code: 128

@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil
Copy link
Contributor

These seem to be real errors, but I don't understand them.

@NancyLi1013
Copy link
Contributor

@strega-nil
Thanks for your help.
The regressions on x64-osx seem like this:

-- Checking file: /Volumes/data/work/1/s/packages/ignition-plugin1_x64-osx/lib/pkgconfig/ignition-plugin1-loader.pc
CMake Error at scripts/cmake/vcpkg_fixup_pkgconfig.cmake:130 (message):
  Library dl was not found! If it is a system library use the
  SYSTEM_LIBRARIES parameter for the vcpkg_fixup_pkgconfig call! Otherwise,
  corret the *.pc file
Call Stack (most recent call first):
  scripts/cmake/vcpkg_fixup_pkgconfig.cmake:232 (vcpkg_fixup_pkgconfig_check_libraries)
  installed/x64-osx/share/ignitionmodularscripts/ignition_modular_library.cmake:29 (vcpkg_fixup_pkgconfig)
  installed/x64-osx/share/ignitionmodularscripts/ignition_modular_library.cmake:131 (ignition_modular_build_library)
  ports/ignition-plugin1/portfile.cmake:4 (ignition_modular_library)
  scripts/ports.cmake:90 (include)

@traversaro
Could you please take a look about this?

@traversaro
Copy link
Contributor Author

Probably it is necessary to pass dl as SYSTEM_LIBRARIES of vcpkg_fixup_pkgconfig, but before we need to expose it in ignition_modular_library, I will prepare a commit to address that.

@traversaro
Copy link
Contributor Author

Probably it is necessary to pass dl as SYSTEM_LIBRARIES of vcpkg_fixup_pkgconfig, but before we need to expose it in ignition_modular_library, I will prepare a commit to address that.

Actually dl is already included in the system libraries in Linux (see https://github.com/microsoft/vcpkg/blob/master/scripts/cmake/vcpkg_fixup_pkgconfig.cmake#L195), probably it make sense to do so also on the other platforms that support the dl library.

@NancyLi1013
Copy link
Contributor

It seems that dl is also required to the system libraries on osx?

@Neumann-A
Could you please help comment this?

@traversaro
Copy link
Contributor Author

It seems that dl is also required to the system libraries on osx?

@Neumann-A
Could you please help comment this?

I investigate a bit and indeed it make sense to have a libdl as system library on macOS, but on the other hand on macOS you don't have libm.
I will update this in the weekend, but basically I was thinking of changing the lines (from https://github.com/microsoft/vcpkg/blob/master/scripts/cmake/vcpkg_fixup_pkgconfig.cmake#L195):

    if(VCPKG_TARGET_IS_LINUX)
        list(APPEND _vfpkg_SYSTEM_LIBRARIES -ldl -lm)
    endif()

to

    # Platforms with libdl
    if(VCPKG_TARGET_IS_LINUX OR VCPKG_TARGET_IS_OSX OR VCPKG_TARGET_IS_ANDROID)
        list(APPEND _vfpkg_SYSTEM_LIBRARIES -ldl)
    endif()
    # Platforms with libm
    if(VCPKG_TARGET_IS_LINUX OR VCPKG_TARGET_IS_FREEBSD OR VCPKG_TARGET_IS_ANDROID)
        list(APPEND _vfpkg_SYSTEM_LIBRARIES -lm)
    endif()

Other platforms used in community triplets probably will also need to be added to this if, but for now I only added the system considered in https://github.com/microsoft/vcpkg/blob/master/scripts/cmake/vcpkg_common_definitions.cmake .

@Neumann-A
Copy link
Contributor

@traversaro:

You can either do that or call vcpkg_fixup_pkgconfig with SYSTEM_LIBRARIES. E.g. vcpkg_fixup_pkgconfig(SYSTEM_LIBRARIES <cmake list of libraries>)

Probably also needs a case for VCPKG_TARGET_IS_MINGW. Maybe it makes more sense to define a global variable called VCPKG_SYSTEM_LIBRARIES in vcpkg_common_definitions? This would allow them to be overwritten globally from the triplet.

@traversaro
Copy link
Contributor Author

Maybe it makes more sense to define a global variable called VCPKG_SYSTEM_LIBRARIES in vcpkg_common_definitions? This would allow them to be overwritten globally from the triplet.

That make totally sense.

@traversaro
Copy link
Contributor Author

Probably also needs a case for VCPKG_TARGET_IS_MINGW.

For which library? As far as I know on MinGW dl is provided by an external library (dlfcn-win32), but indeed I do not know if linking with libm is necessary or not on MinGW.

@Neumann-A
Copy link
Contributor

For which library?

don't know mingw or msys that well. Just thought that there might be a need for an additional case for it as well ;)

@traversaro
Copy link
Contributor Author

@NancyLi1013 @Neumann-A
The PR has been updated to handle system libraries in a more general way, see f38a8cc . This should fix the CI failure on macOS .

@NancyLi1013 Thanks to the comment of @Electric-Turtle in #11273 (comment) I noticed a critical flaw in this PR. Namely, ignition-plugin1 install multiples CMake config files, and it was necessary to modify vcpkg_fixup_cmake_targets (4988fe6) and ignition_modular_library (b587d0f) to handle this case.

@traversaro traversaro mentioned this pull request May 16, 2020
Merged
@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label May 22, 2020
@NancyLi1013
Copy link
Contributor

NancyLi1013 commented May 22, 2020 •
edited
Loading

The recent changes caused some new regressions.
@traversaro
Could you please look into them?

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@traversaro
Copy link
Contributor Author

I may be wrong, but the CI failures seems not to be related with this PR.

@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil
Copy link
Contributor

@traversaro I'll look into this tomorrow

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@traversaro
vcpkg_fixup_cmake_targets: Add DO_NOT_DELETE_PARENT_CONFIG_PATH
190cdbd 
By default the vcpkg_fixup_cmake_targets script remove the parent
path of CONFIG_PATH if it named "cmake", this behaviour is not convenient
for ports that install more than one CMake package config file, and
for which vcpkg_fixup_cmake_targets is invoked multiple times.
To optionally disable this behaviour, this commit adds the option
DO_NOT_DELETE_PARENT_CONFIG_PATH to vcpkg_fixup_cmake_targets.
@traversaro
[ignition-modularscripts] Add support for ignition libraries that ins…
58fd9f4 
…tall multiple CMake package config files

Some ignition libraries install several CMake  package config files,
to represent the different components of the library. This  commit modifies
the ignition_modular_library function to fixup correctly all the cmake package config files.
@traversaro
vcpkg_fixup_pkgconfig: Move definition of SYSTEM_LIBRARIES to vcpkg_c…
83f3459 
…ommon_definitions

To correctly validate installed pkg-config files, vcpkg_fixup_pkgconfig needs to know
for each platform which libraries are not managed by vcpkg. This commits improve this
definitions for all the triplet supported by vcpkg, and move this definition to vcpkg_common_definitions
in a way that permit custom triplets to overload its value.
@traversaro
[ignition-plugin1] Add new port
c564842
@traversaro
Copy link
Contributor Author

PR rebased over latest master.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

All checks seem to be successful.

dan-shaw
dan-shaw reviewed Jun 23, 2020
View reviewed changes
scripts/cmake/vcpkg_fixup_pkgconfig.cmake
@@ -194,8 +194,8 @@ endfunction()
function(vcpkg_fixup_pkgconfig)
cmake_parse_arguments(_vfpkg "" "" "RELEASE_FILES;DEBUG_FILES;SYSTEM_LIBRARIES;SYSTEM_PACKAGES;IGNORE_FLAGS" ${ARGN})

if(VCPKG_TARGET_IS_LINUX)
list(APPEND _vfpkg_SYSTEM_LIBRARIES -ldl -lm)
if(VCPKG_SYSTEM_LIBRARIES)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does pkgconfig need the system libraries on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the system libraries that I have encounter in pkg-config files on Windows are defined in https://github.com/microsoft/vcpkg/pull/11275/files#diff-c1846c91b47ff94ccee5e8a6a1658e62R130 . In particular, I found them in the pcap port (see https://github.com/microsoft/vcpkg/blob/2020.06/ports/libpcap/portfile.cmake#L89), and adding them in a centralized place seems to me consistent to what we are doing in Linux/macOS .

@dan-shaw dan-shaw merged commit 70ab27f into microsoft:master Jun 24, 2020
@dan-shaw
Copy link
Contributor

Thanks for the PR!

@traversaro traversaro deleted the add-ignition-plugin1 branch June 25, 2020 04:38
@traversaro traversaro mentioned this pull request Jul 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dan-shaw dan-shaw dan-shaw approved these changes

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@traversaro @NancyLi1013 @strega-nil @Neumann-A @dan-shaw