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

add make install step to the pod makefile #1970

Merged
merged 3 commits into from
Apr 3, 2016

Conversation

patmarion
Copy link
Member

pod makefiles should build and install

pod makefiles should build and install
@patmarion
Copy link
Member Author

This pull request re-adds the install step to the pod makefile as described here: #1945 (comment)

@jwnimmer-tri
Copy link
Collaborator

Is a way to prevent regressions like this in the future? Another CI target? Add a "sample downstream project" in RobotLocomotion that just links a simple example against Drake, and get that under CI? Some kind of in-build unit-test that looks for the presence of a required pod-build-placed file?

@jamiesnape
Copy link
Contributor

Can openhumanoids wrap Drake as if it were a standard CMake project? Unless I am misreading things in other tickets, it may not stay compatible in the medium to long term.

@patmarion
Copy link
Member Author

@jwnimmer-tri there is a Travis-ci build for openhumanoids. We could setup a robot that automatically updates a branch to test openhumanoids+drake/master. That would be ideal. If you wanted, you could go further and run it for every new drake pull request, instead of after a drake master merge.

@jamiesnape previously, drake's superbuild did not expose enough options for the superbuild to be customized by the embedding project, but that's likely improved. Right now, openhumanoids has it's own superbuild to satisfy dependencies, then it calls the inner drake/drake/Makefile to build just drake. This strategy has its quirks, but it works, and affords flexibility. We'd be open to improvements, for sure!

@patmarion
Copy link
Member Author

assigned to @jwnimmer-tri for review. I just picked you since you were the first to reply :)

@jwnimmer-tri
Copy link
Collaborator

LGTM +1. Still waiting for some builders. Assigning second review & merge to @david-german-tri.

@jhoare
Copy link

jhoare commented Mar 30, 2016

Should this also be added to the root Makefile?

@patmarion
Copy link
Member Author

No, there isn't an install target at the root superbuild currently. A no-op install target could be created, but it's not necessary for the current usage.

@patmarion
Copy link
Member Author

In case anyone is interested, here is some (long) discussion on the topic of building drake in openhumanoids, after the drake superbuild was introduced: #1234

Now that both projects have made more progress, I think it's feasible to implement the approach described here: #1234 (comment)

@david-german-tri
Copy link
Contributor

LGTM +1, sorry for the delay, didn't notice this was assigned to me. Once you sync it should be good to merge.

@RussTedrake RussTedrake merged commit 156954b into RobotLocomotion:master Apr 3, 2016
@wxmerkt
Copy link
Member

wxmerkt commented Apr 5, 2016

@patmarion I recall that after a conference call we've backed down from this idea and decided not to move drake into software/externals because it could accidentally override changes users make inside drake during research/development.

@patmarion
Copy link
Member Author

We could still put it in software/externals, but keep it as a git submodule, and do not allow the CMake ExternalProject call to modify it, that is, skip the update step, but do run the build step.

@patmarion patmarion deleted the fix-pod-makefile branch October 12, 2016 02:33
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.

7 participants