Skip to content

Don't install vendored backward files (take II)#3088

Merged
j-rivero merged 9 commits intogz-sim10from
jrivero/backwards_remove
Sep 29, 2025
Merged

Don't install vendored backward files (take II)#3088
j-rivero merged 9 commits intogz-sim10from
jrivero/backwards_remove

Conversation

@j-rivero
Copy link
Copy Markdown
Contributor

🦟 Bug fix

Retake of #2838

Summary

See #2919 (review).

The PR adds a file for visibility if the version is updated at some point.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers
  • Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Generated-by: Remove this if GenAI was not used.

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by and Generated-by messages.

Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
Copy link
Copy Markdown
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

I guess I'm not sure why we need to even include the CMakeListst.txt of our vendored backward-cpp. Since it's a header-only library, I would think we can just include the header without installing it.

Comment thread vendor/backward-cpp/CMakeLists.txt Outdated
)
# check if Backward is being used as a top-level project or included as a subproject
if(CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR)
install(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I undesrtand this correctly, this would still install the header since it's being included as a subproject in our top level CMakeLists.txt file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CMAKE_SOURCE_DIR should equal to the root of the gz-sim repository while PROJECT_SOURCE_DIR should be equal to the vendor/backwards-cpp subdirectory on top of the root of the gz-sim repository. So we are using it but the file is not a top-level project.

Copy link
Copy Markdown
Contributor

@arjo129 arjo129 Sep 18, 2025

Choose a reason for hiding this comment

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

While the logic make sense, why not completely remove it since we are patching this already?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@j-rivero, I agree. It would be simpler to remove the install calls.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, eb7b252

Jose Luis Rivero and others added 2 commits September 16, 2025 19:40
Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
@azeey azeey mentioned this pull request Sep 27, 2025
7 tasks
@azeey
Copy link
Copy Markdown
Contributor

azeey commented Sep 27, 2025

Looks like the pre-commit test failed. Let's try to get this merged before making the stable release.

Signed-off-by: Carlos Agüero <caguero@osrfoundation.org>
@github-project-automation github-project-automation bot moved this from Inbox to In review in Core development Sep 29, 2025
caguero and others added 2 commits September 29, 2025 17:16
Signed-off-by: Carlos Agüero <caguero@osrfoundation.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Copy Markdown
Member

codespell: skip *.patch files

attempting a fix in 62d8dfc

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Copy Markdown
Member

codespell: skip *.patch files

attempting a fix in 62d8dfc

trying again in bd43185

@scpeters
Copy link
Copy Markdown
Member

codespell: skip *.patch files

attempting a fix in 62d8dfc

trying again in bd43185

sorry this isn't working

fix later

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Copy Markdown
Member

I just removed the whitespace from the patch and reverted my other changes. This makes the patch invalid, so I filed #3096 to record the issue

@j-rivero j-rivero merged commit 1ab1b0b into gz-sim10 Sep 29, 2025
13 of 14 checks passed
@j-rivero j-rivero deleted the jrivero/backwards_remove branch September 29, 2025 19:28
@github-project-automation github-project-automation bot moved this from In review to Done in Core development Sep 29, 2025
iche033 pushed a commit that referenced this pull request Nov 11, 2025
* Back to skip the backwards-cpp vendor files when installing

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
---------

Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
Signed-off-by: Carlos Agüero <caguero@osrfoundation.org>
IceShuttle pushed a commit to IceShuttle/gz-sim that referenced this pull request Nov 14, 2025
* Back to skip the backwards-cpp vendor files when installing

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
---------

Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
Signed-off-by: Carlos Agüero <caguero@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🪵 jetty Gazebo Jetty

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants