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

Declare missing dependency on 'git' #18

Merged
merged 1 commit into from
Jan 25, 2021
Merged

Conversation

cottsay
Copy link
Contributor

@cottsay cottsay commented Jan 21, 2021

Follow-up to #16

Follow-up to f0c78e9

Signed-off-by: Scott K Logan <[email protected]>
@cottsay cottsay added bug Something isn't working in review Waiting for review (Kanban column) labels Jan 21, 2021
@cottsay cottsay requested a review from clalancette January 21, 2021 03:51
@cottsay cottsay self-assigned this Jan 21, 2021
@clalancette
Copy link
Contributor

This one is kind of weird. We don't have a dependency on git for the other places in the codebase that use git-apply (like https://github.com/ros2/tinyxml_vendor/blob/master/package.xml); why is this one different?

@cottsay
Copy link
Contributor Author

cottsay commented Jan 21, 2021

We don't have a dependency on git for the other places in the codebase that use git-apply

This is why this isn't a problem for Ubuntu:
https://github.com/ros-infrastructure/ros_buildfarm/blob/7e1d61fd5fd5608bf85b7db24129aeeb89d5b367/ros_buildfarm/templates/release/deb/binarypkg_task.Dockerfile.em#L58

I haven't gone after tinyxml_vendor because that dependency appears to be satisfied by the system's package on all RedHat platforms, so it doesn't need to build from source. If it did, that one would fail too. Grepping through the ROS 2 codebase, tinyxml_vendor and mimick_vendor are actually the only other packages I can find that are missing this dependency. We do actually declare the dependency on git in foonathan_memory_vendor and rviz_ogre_vendor.

I haven't actually seen this fail on the RPM farm yet because this package hasn't been released for a while, but given that Mimick is failing (ros2/mimick_vendor#16), this package likely will as well.

@cottsay
Copy link
Contributor Author

cottsay commented Jan 22, 2021

I'd like to close on this today, @clalancette, so that I can unblock the RPM builds. If we really want to add git to the list of implicit dependencies, then I'll need to add it to the buildroot in mock along with make and g++. If we go that way, we should probably drop the dependency from other packages so that we're consistent.

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

In general I disagree with implicit dependencies so I'm in favor of this change.

@clalancette
Copy link
Contributor

All right, I'm fine with making this an explicit dependency for now.

Orthogonally, but relatedly, we should probably review how we are vendoring packages. We have some combination of:

  • Using system packages (via apt, homebrew, chocolatey) - examples include asio, libxml2, iconv, zlib
  • Using system packages with vendor override - examples include console_bridge
  • Vendoring by pointing to a specific upstream github repository, then applying patches - examples include uncrustify, mimick
  • Vendoring by forking the upstream repository, applying our own patches, then cloning the repository via ros2.repos - examples include googletest, googlebenchmark

It would be nice to reduce the number of ways we do this, but I guess that is a problem for another day. I'll go ahead and merge this.

@clalancette clalancette merged commit 0a48fdd into master Jan 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the cottsay/git_depend branch January 25, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants