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

[workspace] Flag sdformat, ignition_math, ignition_utils as internal #17303

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jun 1, 2022

We are substantially patching these libraries to weave them into Drake. We should not give the illusion to users that they are available for reuse downstream.

Also take this opportunity to track the ignition => gz project rename.

Towards #17231.


This change is Reviewable

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

+@rpoyner-tri for feature review, please.

Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on @rpoyner-tri)


tools/workspace/deprecation.bzl line 1 at r1 (raw file):

# -*- python -*-

FYI This file is copied from #17229. It will disappear here once we rebase.

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review June 2, 2022 16:44
Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

a discussion (no related file):
Do the ignition_*names in tools/workspace/new_release.py also need to change?

https://github.com/RobotLocomotion/drake/blob/9c9e57540876545172baf70ac955bdb497171227/tools/workspace/new_release.py#L68-L69=


We are substantially patching these libraries to weave them into Drake.
We should not give the illusion to users that they are available for
reuse downstream.

Also take this opportunity to track the ignition => gz project rename.
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

+@sammy-tri for platform review per schedule, please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @rpoyner-tri and @sammy-tri)

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Do the ignition_*names in tools/workspace/new_release.py also need to change?

https://github.com/RobotLocomotion/drake/blob/9c9e57540876545172baf70ac955bdb497171227/tools/workspace/new_release.py#L68-L69=

Done.

Right you are. I also missed qhull_internal in the prior PR, fixed here now as a follow-up.


Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @rpoyner-tri and @sammy-tri)

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

Is there a guideline for when we should be using cc_library vs cc_library_vendored for _internal packages? If I tried to guess the difference between this an qhull_internal, I'd guess that it's something about when the package is only being used by libraries with internal = True, but also the onl;y user of @qhull_internal explicitly marks it as an implementation dependency.

Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @jwnimmer-tri)

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 20 files at r1, 1 of 4 files at r2.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @jwnimmer-tri)

@jwnimmer-tri
Copy link
Collaborator Author

Is there a guideline for when we should be using cc_library vs cc_library_vendored for _internal packages?

It's a good question. In a PR soon, I do plan to update drake/tools/workspace/README with more prescriptive advice.

The simple answer in terms of this PR is that #17230 will add vendoring for all three of these. It just ended up being too much work to do the renames, deprecation, and vendoring all in a single PR, so I carved this one out first.

More broadly, it's a one-way implication: anything that's vendored should be marked _internal, but it's OK to have internal repositories that don't use vendoring. For example, @dm_control is just a bunch of model files used for our unit testing. I don't want that to be part of our Stable API policy, so I plan to rename it to @dm_control_internal, but it won't use C or C++ vendoring.

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: I'll let you race #17322 for who gets the merge commit.

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sammy-tri(platform) (waiting on @jwnimmer-tri)

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

err, merge conflict

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sammy-tri(platform) (waiting on @jwnimmer-tri)

@jwnimmer-tri
Copy link
Collaborator Author

Hmm, that one already needs a rebase anyway, so I'll just go first.

@jwnimmer-tri jwnimmer-tri merged commit 1ade0c1 into RobotLocomotion:master Jun 2, 2022
@jwnimmer-tri jwnimmer-tri deleted the rename-ignition branch June 2, 2022 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: newly deprecated This pull request contains new deprecations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants