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

Restore "workspace: Update pybind11 fork to latest commit" #14048

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Sep 11, 2020

Downstream Warning:
If you are using CMake to consume pydrake and you compile your own bindings using pybind11_add_module, you must now reset your target's default visibility to "default" (not "hidden").
See drake_cmake_installed for an example of doing so.

This reverts commit 690a7ea.

Resolves #14047


This change is Reviewable

@EricCousineau-TRI EricCousineau-TRI added the status: single reviewer ok https://drake.mit.edu/reviewable.html label Sep 11, 2020
Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

+@jamiesnape for both reviews, please
+(status: single reviewer ok)

Reviewable status: LGTM missing from assignee jamiesnape, needs platform reviewer assigned (waiting on @jamiesnape)

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a 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 assignee jamiesnape, needs platform reviewer assigned (waiting on @EricCousineau-TRI and @jamiesnape)

a discussion (no related file):
Working: Should merge once drake-external-examples PR merges first.


Copy link
Contributor

@jamiesnape jamiesnape 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 1 of 1 files at r1.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned (waiting on @EricCousineau-TRI)

@jwnimmer-tri
Copy link
Collaborator

If the required fix is to downstream code, then please add release notes text into this commit message to warn users about the breaking change.

@EricCousineau-TRI
Copy link
Contributor Author

Good point - will do. Thanks!

@EricCousineau-TRI EricCousineau-TRI self-assigned this Sep 11, 2020
Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Self-assigning platform (I think this works?): +@EricCousineau-TRI :lgtm:

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees EricCousineau-TRI(platform),jamiesnape

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Should merge once drake-external-examples PR merges first.

Done.


@EricCousineau-TRI EricCousineau-TRI merged commit 1c89da9 into RobotLocomotion:master Sep 11, 2020
@DamrongGuoy
Copy link
Contributor

Apologize if I misunderstood. I thought @jwnimmer-tri suggested that @EricCousineau-TRI added a warning into commit message, but I do not see it in GitHub commit. It looks like this:

image.png

I'm sorry if I wasn't look at the right place.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Sep 13, 2020

Ah, sorry Damrong! I had thought that we copied the existing overview text, but forgot that we did not and pushed the Squash-and-merge button too fast.

For simplicity, I can add a minor no-op commit that incorporates this text, if it makes it easier for commit analysis for release notes?

@jwnimmer-tri
Copy link
Collaborator

In general, for release note amendments other than the commit message, you can either:

  • push a commit to the release notes PR branch, or
  • add a review comment to the release notes PR with something to copy-paste.

I'll add the PR overview text into the notes momentarily, though.

@EricCousineau-TRI
Copy link
Contributor Author

Awesome, thanks!

@jamiesnape jamiesnape removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pybind: Should restore fork's latest commit in workspace pending mac fix
4 participants