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

[gz-ports, sdformat] Update gz-ports #43265

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

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Jan 14, 2025

  • 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.

Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
Signed-off-by: Tal Regev <[email protected]>
@talregev talregev force-pushed the TalR/gz_ports branch 4 times, most recently from d3686fa to e4262db Compare January 14, 2025 23:02
@talregev talregev force-pushed the TalR/gz_ports branch 2 times, most recently from 6c68c31 to 2b2dc74 Compare January 17, 2025 07:48
@WangWeiLin-MV WangWeiLin-MV added the category:port-update The issue is with a library, which is requesting update new revision label Jan 17, 2025
Signed-off-by: Tal Regev <[email protected]>
@talregev talregev marked this pull request as ready for review January 17, 2025 11:00
@talregev
Copy link
Contributor Author

@WangWeiLin-MV @LilyWangLL
The PR is ready for review

Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a comment

Choose a reason for hiding this comment

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

Take gz-cmake* as an example.

There are 4 ports: 3 ports named ignition-cmake0, ignition-cmake2, gz-cmake3 in vcpkg/ports, and 1 port gz-cmake4 in this PR. The home page of all these ports linked to https://ignitionrobotics.org/libs/cmake (Will redirect to https://gazebosim.org/libs/cmake/)

Although the exported package names do not conflict, ignition-cmake0, ignition-cmake2, gz-cmake3, should we only maintain the latest version and rename it to the name without version?

@vicroms Waiting for further review.

@talregev
Copy link
Contributor Author

talregev commented Jan 22, 2025

@WangWeiLin-MV Can you put requires:vcpkg-team-review label that the team will review it?

@WangWeiLin-MV
Copy link
Contributor

Waiting for further review.
@vicroms @BillyONeal

@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Feb 1, 2025
@@ -0,0 +1,200 @@
diff --git a/cmake/FindFreeImage.cmake b/cmake/FindFreeImage.cmake
index 8a5836e..7f926cc 100644
--- a/cmake/FindFreeImage.cmake
Copy link
Member

Choose a reason for hiding this comment

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

Review note: This file is substantially identical to the gz-cmake3 one with the tinyxml block moved around.

No change requested.

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.

This comments were never sent... Maybe I never finished the review.

Comment on lines +10 to +11
+if(FreeImage_FOUND)
+ # done
Copy link
Contributor

Choose a reason for hiding this comment

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

Never skip.

"${OGRE2LIBNAME}${component}.${OGRE2_VERSION}"
"${OGRE2LIBNAME}${component}"
+ "${OGRE2LIBNAME}${component}Static"
+ "${OGRE2LIBNAME}${component}Static_d"
Copy link
Contributor

Choose a reason for hiding this comment

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

NAMES_PER_DIR nearby?

- list(APPEND OGRE2_PATHS "${_rootPath}/lib/${OGRE2_SEARCH_VER}/manual-link/")
- list(APPEND OGRE2_INC_PATHS "${_rootPath}/include/${OGRE2_SEARCH_VER}")
+ get_filename_component(debug_dir "${_rootPath}" NAME)
+ if(debug_dir MATCHES "debug" AND CMAKE_BUILD_TYPE MATCHES Debug OR NOT debug_dir MATCHES "debug" AND CMAKE_BUILD_TYPE MATCHES Release)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the intention of this condition?

@@ -0,0 +1,3 @@
The package gz-cmake4 provides CMake integration:

find_package(gz-cmake4 CONFIG REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

This not showing any actual use.

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Feb 4, 2025
@BillyONeal
Copy link
Member

BillyONeal commented Feb 4, 2025

Take gz-cmake* as an example.

I agree that this is ... unusual but we have accepted it in the past, as the gz-Xxx and ignition-Xxx appear to intentionally support simultaneous versions. I think we should continue letting contributors follow a pattern they've been following for years unless we have strong reason to change how something works. That said,

@talregev What do you think about dropping the version number suffixes and maintaining only one set we know work together? Or do you know of customers who specifically need many simultaneous versions necessitating so many duplicates?

@BillyONeal
Copy link
Member

arm64-osx looks like a transient network issue

@talregev
Copy link
Contributor Author

talregev commented Feb 4, 2025

What do you think about dropping the version number suffixes and maintaining only one set we know work together?

I am in favor to dropping the version number suffixes, and only in needed cases we can add the one alongside with version number suffixes.

But I cannot do it alone. We need to fix gz-common6 first, currently I didn't succeeded to compile it on osx and linux.
and it very basic in the gz ports, and it will limited heavily other ports.
https://github.com/microsoft/vcpkg/pull/43265/files#diff-a1a3210f49402878917bb13f06dfe89def10114b731dfb51d3e16c1a1f563995R7

Also we should delist port gazebo. this is a gazebo classic, that ended it life:
https://github.com/gazebosim/gazebo-classic/

And we can try in other PR to add the current gazebo as gz-sim port:
https://github.com/gazebosim/gz-sim

@talregev
Copy link
Contributor Author

talregev commented Feb 4, 2025

@dg0yt Thank you for your comments.
All your comments, the code was taken from gz-cmake3 code. It a code that I copy and modify for gz-cmake4.

@dg0yt
Copy link
Contributor

dg0yt commented Feb 4, 2025

All your comments, the code was taken from gz-cmake3 code. It a code that I copy and modify for gz-cmake4.

Yeah, bad examples proliferate.

@talregev
Copy link
Contributor Author

talregev commented Feb 4, 2025

All your comments, the code was taken from gz-cmake3 code. It a code that I copy and modify for gz-cmake4.

Yeah, bad examples proliferate.

Sorry, I am not sure how to fix it. Can you suggest fixes, and I will apply them?

@dg0yt
Copy link
Contributor

dg0yt commented Feb 4, 2025

Can you suggest fixes, and I will apply them?

None of the three review comment needs extra suggestion at the moment.

@BillyONeal BillyONeal marked this pull request as draft February 5, 2025 04:40
@BillyONeal
Copy link
Member

The conflict between gz-gui7 and gz-gui9 looks like a real conflict.

I am in favor to dropping the version number suffixes, and only in needed cases we can add the one alongside with version number suffixes.

But I cannot do it alone. We need to fix gz-common6 first, currently I didn't succeeded to compile it on osx and linux. and it very basic in the gz ports, and it will limited heavily other ports. https://github.com/microsoft/vcpkg/pull/43265/files#diff-a1a3210f49402878917bb13f06dfe89def10114b731dfb51d3e16c1a1f563995R7

Also we should delist port gazebo. this is a gazebo classic, that ended it life: https://github.com/gazebosim/gazebo-classic/

And we can try in other PR to add the current gazebo as gz-sim port: https://github.com/gazebosim/gz-sim

Tagging other folks who have touched the ignitionrobotics stuff in the past:

@traversaro
@ahoarau
@Sibras
@RobbertProost

What are your thoughts about this situation?

@traversaro
Copy link
Contributor

traversaro commented Feb 5, 2025

What are your thoughts about this situation?

First of all, I am a happy user of vcpkg for other projects, so I am quite sorry for creating the problem in the first place and then stopping using vcpkg for ignition/gazebo packages. If you are interested in a bit of history, the initial ports (using major version in the name) of ignition/gazebo packages were added in #7781 . The rationale for doing that was part of the original description of the PR.

The possibility of installing side-by-side multiple major version of ignition/gazebo packages in the same system/project/environment (depending on weather you are using a system/project/environment package manager) was quite convenient when both Gazebo Classic and gz-sim (i.e "Modern Gazebo") were supported and used different major versions of their dependencies. However, now that Gazebo Classic reached its EOL (the repo was archived: https://github.com/gazebosim/gazebo-classic) it is not so useful anymore, and even upstream is considering dropping this possibility: gazebo-tooling/release-tools#1244 .

For these reasons, my answer to:

What do you think about dropping the version number suffixes and maintaining only one set we know work together? Or do you know of customers who specifically need many simultaneous versions necessitating so many duplicates?

I think it make perfect sense to drop all the version number suffixes and just package the latest major version of each gz library.

@@ -0,0 +1,3 @@
The package gz-cmake4 provides CMake integration:

find_package(gz-cmake4 CONFIG REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking as example

list(INSERT CMAKE_MODULE_PATH 0 ${ECM_MODULE_PATH})
, I guess an example of actual usage (inspired from https://github.com/gazebosim/gz-cmake/blob/ceee29190f18847687d1a383c6f6b1219ce95744/examples/gz_conf/CMakeLists.txt) is:

Suggested change
find_package(gz-cmake4 CONFIG REQUIRED)
find_package(gz-cmake4 CONFIG REQUIRED)
gz_configure_project()
gz_configure_build(QUIT_IF_BUILD_ERRORS)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants