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

Fix PopulateUpstreamToDrake for checkout not named drake #7864

Merged
merged 1 commit into from
Jan 26, 2018

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 25, 2018

The prior version relied on "/drake/" appearing in absolute pathnames to URDF files, then looked for package.xml files in the trip from the URDF down to drake's root. This worked when all resources were in drake-distro/drake. When we got rid of drake-distro, it still worked if your git clone was named drake, or if you were running a test in the bazel sandbox where the workspace is named drake. If you were running from the command line, it would give up because it didn't know where Drake was.

The fix is to use GetDrakePath to establish the coordinate to search up until. This still might fail if the URDF is from some foreign Drake tree, but GetDrakePath is finding the current tree (it won't add the foreign packages), but at least this is an improvement.

Fixes #7855. Relates #6996.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@soonho-tri for all review please. I will find someone to test locally also.

@soonho-tri
Copy link
Member

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


multibody/parsers/package_map.cc, line 135 at r1 (raw file):

void PackageMap::PopulateUpstreamToDrakeHelper(
    const string& directory,
    const std::string& stop_at_directory) {

BTW, std:: can be removed.


multibody/parsers/package_map.cc, line 139 at r1 (raw file):

  // If we've reached the top, then stop searching.
  if (directory.length() <= stop_at_directory.length()) {

BTW, this only works if we know that stop_at_directory is an ancestor of directory. Do we have this as a pre-condition?

OK.


multibody/parsers/package_map.h, line 82 at r1 (raw file):

  // Recursively searches up the directory path searching for package.xml files.
  // Adds the packages defined by these package.xml files to this PackageMap.
  // This method is intended to be called by PopulateUpstreamToDrake().

BTW, need to update the doc section?


Comments from Reviewable

The prior version relied on "/drake/" appearing in absolute pathnames to
URDF files, then looked for package.xml files in the trip from the URDF
down to drake's root.  This worked when all resources were in
drake-distro/drake.  When we got rid of drake-distro, it still worked if
your git clone was named drake, or if you were running a test in the
bazel sandbox where the workspace is named drake.  If you were running
from the command line, it would give up because it didn't know where
drake was.

The fix is to use GetDrakePath to establish the coordinate to search up
until.  This still might fail if the URDF is from some foreign Drake
tree, but GetDrakePath is finding the current tree (it won't add the
foreign packages), but at least this is an improvement.
@soonho-tri
Copy link
Member

Reviewed 2 of 2 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@soonho-tri
Copy link
Member

I will find someone to test locally also.

Good to merge this as @rdeits tested it?


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@jwnimmer-tri jwnimmer-tri merged commit ebcf502 into RobotLocomotion:master Jan 26, 2018
@jwnimmer-tri jwnimmer-tri deleted the packagemap branch January 26, 2018 01:34
@jwnimmer-tri
Copy link
Collaborator Author

Hmm, this had the side-effect that merely loading URDFs requires that FindResource works now, even if the user never wants or tries to use FindResource to locate URDFs. I will try to follow-up and weaken that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package map fails when Drake is cloned to a folder with name other than "drake"
2 participants