Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[ignition-common3] Add new port 🤖 #11273

Merged
merged 2 commits into from
Jan 5, 2021

Conversation

traversaro
Copy link
Contributor

  • What does your PR fix?

    • This PR adds a new port for the new major version of the ignition-common library.
  • Which triplets are supported/not supported?

    • All tested triplet (for which the dependency are actually available) 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-common3 is a dependency for Gazebo 11 (http://gazebosim.org/blog/gazebo11) and for 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-common3] Add new port [ignition-common3] Add new port 🤖 May 9, 2020
@traversaro traversaro force-pushed the add-ignition-common3 branch from 01a7635 to aae7377 Compare May 9, 2020 15:54
@PhoebeHui PhoebeHui self-assigned this May 11, 2020
Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

@PhoebeHui
Copy link
Contributor

PhoebeHui commented May 12, 2020

@traversaro, thanks for quick updates!

I noticed ignition-common3:x64-linux failed on linux CI testing, could you have a look?

CMake Error at /mnt/1/s/installed/x64-linux/share/cmake/ignition-cmake2/cmake2/IgnConfigureBuild.cmake:66 (message):
Errors encountered in build. Please see BUILD ERRORS above.
Call Stack (most recent call first):
CMakeLists.txt:120 (ign_configure_build)

config-x64-linux-dbg-err.log
config-x64-linux-dbg-out.log

@traversaro traversaro force-pushed the add-ignition-common3 branch from 96416df to 41bf8a6 Compare May 12, 2020 22:17
@TrentWeiss
Copy link
Contributor

Something I have noticed:

This port doesn't properly install the cmake config files for ignition-common3's various subcomponents, ( "graphics", "profiler", etc). I.e. the various -config.cmake and -config-version.cmake never make it to the vcpkg installation directory. However, the actual library files do get built and installed.

for example:
C:\path\to\vcpkginstalldir\lib\ignition-common3-graphics.lib is present after the install finishes, but the corresponding cmake config file that tells cmake where to find it does not.
I.e. C:\path\to\vcpkginstalldir\share\ignition-common3\ignition-common3-graphics-config.cmake doesn't exist. If you try to do a find_package for ignition-common3 with graphics specified as a component such as the following:
find_package(ignition-common3 CONFIG REQUIRED
COMPONENTS
graphics
)
The find_package call will fail, even though the library was technically built. For example, if you tried to build Gazebo11 (which I strongly sense is the motivation for this port), the config stage will not work. The cmake configuration of Gazebo will THINK ignition-common3-graphics doesn't exist, because the config file for it is nowhere to be found.

@traversaro
Copy link
Contributor Author

The find_package call will fail, even though the library was technically built. For example, if you tried to build Gazebo11 (which I strongly sense is the motivation for this port), the config stage will not work. The cmake configuration of Gazebo will THINK ignition-common3-graphics doesn't exist, because the config file for it is nowhere to be found.

Thanks for the comment. For the moment I am actually building Gazebo11 by building all the ignition related dependencies with colcon (see https://github.com/robotology/robotology-superbuild-dependencies-vcpkg/blob/v0.3.0/.github/workflows/ci.yml#L171), but indeed the main motivation of brinding all the libraries to vcpkg is to be able to avoid to rely on colcon.

@TrentWeiss
Copy link
Contributor

Thanks for the comment. For the moment I am actually building Gazebo11 by building all the ignition related dependencies with colcon (see https://github.com/robotology/robotology-superbuild-dependencies-vcpkg/blob/v0.3.0/.github/workflows/ci.yml#L171), but indeed the main motivation of brinding all the libraries to vcpkg is to be able to avoid to rely on colcon.

Haha I am currently doing the exact same thing and would also like to avoid having to maintain a custom build of Gazebo with colcon.

@traversaro traversaro force-pushed the add-ignition-common3 branch from 41bf8a6 to deeb523 Compare May 16, 2020 15:02
@traversaro
Copy link
Contributor Author

@traversaro, thanks for quick updates!

I noticed ignition-common3:x64-linux failed on linux CI testing, could you have a look?

CMake Error at /mnt/1/s/installed/x64-linux/share/cmake/ignition-cmake2/cmake2/IgnConfigureBuild.cmake:66 (message):
Errors encountered in build. Please see BUILD ERRORS above.
Call Stack (most recent call first):
CMakeLists.txt:120 (ign_configure_build)

config-x64-linux-dbg-err.log
config-x64-linux-dbg-out.log

I pushed an updated version of the branch that should fix it by backporting the ignition-cmake PR gazebosim/gz-cmake#95 .

@traversaro
Copy link
Contributor Author

traversaro commented May 16, 2020

Something I have noticed: [...]

This has been fixed in #11275 , and the fix will automatically work also for the ignition-common3 port as soon as that PR is merged.

@traversaro
Copy link
Contributor Author

Hi @PhoebeHui , as I see this PR listed under "requires:author-response", if there is anything else that needs my input feel free to ask, thanks!

@seanyen
Copy link
Contributor

seanyen commented Jun 6, 2020

wow, looks like we are about to get all deps for Gazebo11. 😉

@traversaro traversaro force-pushed the add-ignition-common3 branch from 1b3bc3d to b022625 Compare June 6, 2020 08:43
@traversaro
Copy link
Contributor Author

PR rebased over latest master, and all the changes requested by the reviewer have been addressed.

@traversaro
Copy link
Contributor Author

There are a few failures in the x64-linux triplet, in particular it is not able to find anymore uuid, FreeImage and gts :

-- BUILD WARNINGS
-- 	Cannot build component [graphics] - Missing: FreeImage
-- 	Cannot build component [graphics] - Missing: gts - GNU Triangulation Surface library
-- END BUILD WARNINGS

-- BUILD ERRORS: These must be resolved before compiling.
-- 	Missing: uuid
-- END BUILD ERRORS

CMake Error at /mnt/vcpkg-ci/install/x64-linux/share/cmake/ignition-cmake2/cmake2/IgnConfigureBuild.cmake:66 (message):
  Errors encountered in build.  Please see BUILD ERRORS above.
Call Stack (most recent call first):
  CMakeLists.txt:120 (ign_configure_build)

I guess this is probably due to the fact that now pkg-config is installed in CI machines, and the .pc files installed by those ports are not correct/well formed.

@traversaro
Copy link
Contributor Author

There are a few failures in the x64-linux triplet, in particular it is not able to find anymore uuid, FreeImage and gts :

-- BUILD WARNINGS
-- 	Cannot build component [graphics] - Missing: FreeImage
-- 	Cannot build component [graphics] - Missing: gts - GNU Triangulation Surface library
-- END BUILD WARNINGS

-- BUILD ERRORS: These must be resolved before compiling.
-- 	Missing: uuid
-- END BUILD ERRORS

CMake Error at /mnt/vcpkg-ci/install/x64-linux/share/cmake/ignition-cmake2/cmake2/IgnConfigureBuild.cmake:66 (message):
  Errors encountered in build.  Please see BUILD ERRORS above.
Call Stack (most recent call first):
  CMakeLists.txt:120 (ign_configure_build)

I guess this is probably due to the fact that now pkg-config is installed in CI machines, and the .pc files installed by those ports are not correct/well formed.

Actually there was a missing dependency in the CONTROL file. This should have been fixed in the last commit.

@traversaro traversaro force-pushed the add-ignition-common3 branch from 9f7dc44 to ee88ccb Compare June 7, 2020 13:15
@traversaro
Copy link
Contributor Author

traversaro commented Jun 7, 2020

I also backported gazebosim/gz-common#71 to avoid having Windows failures. There is still a x64_osx failure, but it is not related to the PR. Windows CI still fails, but with the unrelated failure in the port qscintilla:x86-windows .

@traversaro traversaro force-pushed the add-ignition-common3 branch from ee88ccb to 74a56f1 Compare June 7, 2020 13:45
@traversaro
Copy link
Contributor Author

The linux failure is due to #11817, so for now I just updated the baseline.

@traversaro traversaro force-pushed the add-ignition-common3 branch from 74a56f1 to 3118e16 Compare June 7, 2020 14:32
@traversaro
Copy link
Contributor Author

that again seems something that will be solved by #12326, that itself is blocked by #11550 and #11776 .

Status update:

@traversaro
Copy link
Contributor Author

that again seems something that will be solved by #12326, that itself is blocked by #11550 and #11776 .

Status update:

* #11550 has been merged

* #11776 still needs to be merged, blocked by #11836 and #12405

* #12326 still needs to be merged, blocked by #11776

Status update:

@traversaro
Copy link
Contributor Author

I opened a separate issue for the ffmpeg/.pc files problem as it is not trivial: #12376 .

This will be fixed (hopefully) by #13919 .

@traversaro
Copy link
Contributor Author

Status update:

* #11776 still needs to be merged, still blocked by #12936

* #12326 still needs to be merged, still blocked by #11776

* #13919 still needs to be merged, possibly blocked by #13126 and #9966 depending on the outcome of the discussion in the PR

New status update:

@traversaro
Copy link
Contributor Author

Status updated:

  • Cleanup the PR as several changes are not necessary anymore due to updated to ignition ports
  • Bumped used version to 3.9.0

#13919 still needs to be merged, possibly blocked by #13126 and #9966 depending on the outcome of the discussion in the PR

This was superseded by #15127 .

The Linux CI is now blocked by #15326, that I guessed could be solved by #12326 but probably can be solved also by other glib-related PRs.

@traversaro traversaro force-pushed the add-ignition-common3 branch from e9f95a8 to 28aaad0 Compare January 2, 2021 23:19
@traversaro traversaro marked this pull request as ready for review January 2, 2021 23:41
@traversaro
Copy link
Contributor Author

traversaro commented Jan 2, 2021

Thanks to #15360, the PR is now passing all the CI tests and is now ready for review @PhoebeHui .

fyi @seanyen @ahoarau

@PhoebeHui PhoebeHui added requires:author-response and removed depends:different-pr This PR or Issue depends on a PR which has been filed labels Jan 5, 2021
Co-authored-by: Phoebe <20694052+PhoebeHui@users.noreply.github.com>
Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

Thanks for your updates!

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Jan 5, 2021
@vicroms vicroms merged commit d2e0939 into microsoft:master Jan 5, 2021
@vicroms
Copy link
Member

vicroms commented Jan 5, 2021

Thanks for the PR!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants