-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[parsing] Deprecate PopulateUpstreamToDrake #16434
[parsing] Deprecate PopulateUpstreamToDrake #16434
Conversation
\CC @EricCousineau-TRI @cottsay FYI +@sammy-tri for feature review, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),sammy-tri(platform) (waiting on @jwnimmer-tri and @rpoyner-tri)
multibody/parsing/parser.cc, line 47 at r1 (raw file):
// Always search for a package.xml file, starting the crawl upward from // the file's path. package_map_.PopulateUpstreamToDrake(file_name);
If this function is a no-op now, why not delete the call instead of ignoring the deprecation? (and if it's still doing something, that should probably be explained somewhere)
6aa2b9a
to
a1d134d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),sammy-tri(platform) (waiting on @rpoyner-tri and @sammy-tri)
multibody/parsing/parser.cc, line 47 at r1 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
If this function is a no-op now, why not delete the call instead of ignoring the deprecation? (and if it's still doing something, that should probably be explained somewhere)
Done.
The deprecation comment was somewhat imprecise; I've amended it.
In cases where the user is relying on a deprecated package.xml file that lives within Drake directly (e.g., [1]), it is not a no-op. In any other case, it is a no-op. For now, the parser needs to backfill this search path, but once those files are removed, we can remove this line (even while leaving the deprecated function intact until 05-01).
[1] For example:
drake/manipulation/models/franka_description/package.xml
Lines 15 to 19 in a064a74
<deprecated> | |
The 'package://franka_description' URI spelling is deprecated, and will be | |
removed from Drake on or around 2022-03-01. Please spell the URI as | |
'package://drake/manipulation/models/franka_description' instead. | |
</deprecated> |
+@rpoyner-tri for platform review per schedule, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform) (waiting on @rpoyner-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 12 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sammy-tri(platform) (waiting on @jwnimmer-tri)
Part of #10531.
This should have been concomitant with #15947, but we overlooked it. Since we don't have any more non-root manifests (outside of unit tests), we don't need any function or automation to find them.
This change is