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

Switch to ignition-math4. #7622

Merged
merged 2 commits into from
Jan 4, 2018
Merged

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Dec 15, 2017

This is going to be needed to move to newer versions of ignition
libraries, newer sdformat, and for delphyne going forward. While
we are at it, make ignition-math a shared library.

Signed-off-by: Chris Lalancette [email protected]


This change is Reviewable

@clalancette
Copy link
Contributor Author

@caguero FYI

@nuclearsandwich
Copy link
Contributor

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please

@clalancette
Copy link
Contributor Author

The macOS failures here were caused by #7439 , and fixed by 96d1681 , so unrelated to this PR.

@clalancette
Copy link
Contributor Author

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please

@clalancette
Copy link
Contributor Author

Rebased onto the latest.

@clalancette
Copy link
Contributor Author

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please

@clalancette
Copy link
Contributor Author

@drake-jenkins-bot retest this please

@clalancette
Copy link
Contributor Author

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please

@clalancette
Copy link
Contributor Author

@drake-jenkins-bot linux-xenial-clang-bazel-experimental-debug please
@drake-jenkins-bot linux-xenial-clang-bazel-experimental-memcheck-lsan please

@jamiesnape
Copy link
Contributor

+@jamiesnape for feature review.

@jamiesnape jamiesnape self-assigned this Dec 20, 2017
@jamiesnape
Copy link
Contributor

:lgtm:


Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please
@drake-jenkins-bot mac-sierra-clang-cmake-experimental-matlab please

@jamiesnape
Copy link
Contributor

+@ggould-tri for platform review.

@ggould-tri
Copy link
Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion.


tools/workspace/ignition_math/ignition_math.BUILD.bazel, line 54 at r1 (raw file):

)

public_headers_no_gen = [

I'm confused by this name. Why do we call it no_gen but then add to it the Export.hh that we just genruled?


Comments from Reviewable

This is going to be needed to move to newer versions of ignition
libraries, newer sdformat, and for delphyne going forward.  While
we are at it, make ignition-math a shared library.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tools/workspace/ignition_math/ignition_math.BUILD.bazel, line 54 at r1 (raw file):

Previously, ggould-tri wrote…

I'm confused by this name. Why do we call it no_gen but then add to it the Export.hh that we just genruled?

Yeah, good point. I'll move the Export.hh out of there and into the list of generated files.


Comments from Reviewable

@clalancette
Copy link
Contributor Author

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please
@drake-jenkins-bot mac-sierra-clang-cmake-experimental-matlab please

@ggould-tri
Copy link
Contributor

:lgtm:


Reviewed 2 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@clalancette
Copy link
Contributor Author

This passed review, and passes all tests. I'm not sure what the merge procedure is (@jwnimmer-tri has merged all of my PRs in the past). Should I just go ahead and merge it?

@ggould-tri
Copy link
Contributor

If the merge button in the upper right is green, go ahead and hit it.
If for some reason it is not, let me know and I will take care of it for you.


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


Comments from Reviewable

@clalancette
Copy link
Contributor Author

I see the green button, so I'll merge. Thanks @ggould-tri .


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


Comments from Reviewable

@clalancette clalancette merged commit fb6d777 into RobotLocomotion:master Jan 4, 2018
@clalancette clalancette deleted the ignition-math4 branch January 4, 2018 13:58
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants