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

Remove (most) examples from CMake build #6441

Merged
merged 1 commit into from
Jun 28, 2017
Merged

Remove (most) examples from CMake build #6441

merged 1 commit into from
Jun 28, 2017

Conversation

jamiesnape
Copy link
Contributor

@jamiesnape jamiesnape commented Jun 27, 2017

Toward #3129. Leaving Atlas (used by drake/systems) and kuka_iiwa_arm (used by spartan).


This change is Reviewable

@jamiesnape
Copy link
Contributor Author

+@jwnimmer-tri for feature review.

@jamiesnape jamiesnape changed the title Remove examples from CMake build Remove (most) examples from CMake build Jun 28, 2017
@jwnimmer-tri
Copy link
Collaborator

:lgtm:


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


a discussion (no related file):
What's the plan for Bazel possibly installing the models (i.e., data files) from examples?


a discussion (no related file):
I need to double-check that all of this code has BUILD files in place. Will post back here soon.


drake/examples/CMakeLists.txt, line 2 at r1 (raw file):

drake_install_headers(
    examples_package_map.h)

This file is dead code and is not known to Bazel. Can we either remove it in this PR, or elsewhere track that we need to remove it?


drake/examples/kuka_iiwa_arm/CMakeLists.txt, line 104 at r1 (raw file):

add_subdirectory(controlled_kuka)
add_subdirectory(dev)

I'm not sure why only this one thing gets removed? Is this per some kind of separate discussion with the kuka team?


Comments from Reviewable

@jamiesnape
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 4 unresolved discussions.


drake/examples/CMakeLists.txt, line 2 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This file is dead code and is not known to Bazel. Can we either remove it in this PR, or elsewhere track that we need to remove it?

Sure.


drake/examples/kuka_iiwa_arm/CMakeLists.txt, line 104 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I'm not sure why only this one thing gets removed? Is this per some kind of separate discussion with the kuka team?

Actually, all the subdirectories should go. Will fix.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

drake/examples/kuka_iiwa_arm/CMakeLists.txt, line 104 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Actually, all the subdirectories should go. Will fix.

Still unclear on why only some of the CMake goes and some stays (i.e., what's the dividing line). Hold-over glue for Spartan transition?


Comments from Reviewable

@jamiesnape
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 4 unresolved discussions.


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

What's the plan for Bazel possibly installing the models (i.e., data files) from examples?

Sure, unless the plan has changed from the days of CMake, @RussTedrake wanted all the examples to be part of redistributable packages..


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Review status: all files reviewed at latest revision, 3 unresolved discussions.


a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

Sure, unless the plan has changed from the days of CMake, @RussTedrake wanted all the examples to be part of redistributable packages..

Should we block CMakeLists turn-down on verifying that an appropriate install dependency is listed in //:BUILD.bazel? The example models don't seem to be there yet.


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I need to double-check that all of this code has BUILD files in place. Will post back here soon.

All good; drake/examples is fully-covered by Bazel, other than the examples_package_map.h already noted in a different discussion.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 6 of 7 files at r2.
Review status: 39 of 40 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@jamiesnape
Copy link
Contributor Author

Review status: 39 of 40 files reviewed at latest revision, 2 unresolved discussions.


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Should we block CMakeLists turn-down on verifying that an appropriate install dependency is listed in //:BUILD.bazel? The example models don't seem to be there yet.

We could, though in practice, CMake did not package them either. I can add something to this PR.


drake/examples/kuka_iiwa_arm/CMakeLists.txt, line 104 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Still unclear on why only some of the CMake goes and some stays (i.e., what's the dividing line). Hold-over glue for Spartan transition?

Exactly: https://github.com/RobotLocomotion/spartan/blob/master/cmake/modules/FindDrake.cmake#L29


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

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


a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

We could, though in practice, CMake did not package them either. I can add something to this PR.

OK I'm satisfied either way here, as long as there is some rationale for where we cross-check that everything is installed -- either incrementally in the CMake turn-down PRs, or in an "install all the things" PR based on bazel query were we just do a top-to-bottom inspection of the install list.


drake/examples/kuka_iiwa_arm/CMakeLists.txt, line 104 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Exactly: https://github.com/RobotLocomotion/spartan/blob/master/cmake/modules/FindDrake.cmake#L29

OK I'm convinced. Can you be sure to ping slack #drake-kuka-iiwa to make sure they are ready for this PR?


Comments from Reviewable

@jamiesnape
Copy link
Contributor Author

+@ggould-tri for platform review.

@ggould-tri
Copy link
Contributor

:lgtm:


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


Comments from Reviewable

@jamiesnape jamiesnape merged commit 0b0fc0e into RobotLocomotion:master Jun 28, 2017
@jamiesnape jamiesnape deleted the cmake-remove-examples branch June 28, 2017 18:24
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.

3 participants