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.

Sign up for GitHub

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

Jump to bottom

Remove PODS Makefile #2809

Merged
merged 2 commits into from
Jul 18, 2016
Merged

Remove PODS Makefile #2809

merged 2 commits into from
Jul 18, 2016

Conversation

mwoehlke-kitware
Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware commented Jul 14, 2016 •
edited by david-german-tri
Loading

Remove the top-level Makefile that was included for PODS compatibility. Users of drake should build drake as a 'typical' CMake project.

This change is Reviewable

@mwoehlke-kitware
Remove PODS Makefile
2c1ca98 
Remove the top-level Makefile that was included for PODS compatibility.
Users of drake should build drake as a 'typical' CMake project.
@mwoehlke-kitware mwoehlke-kitware mentioned this pull request Jul 14, 2016
Closed
@jhoare
Copy link

jhoare commented Jul 14, 2016

Should remove drake/Makefile as well.

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@liangfok
Copy link
Contributor

I believe this comment prevents us from removing drake-distro/drake/Makefile: #2484 (comment)

Basically, to support downstream users who are currently using Drake as a POD within their project.

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@david-german-tri
Copy link
Contributor

Hold on. We definitely need to preserve the ability for downstream projects to use Drake as a POD. Based on #2484, my understanding was that Makefile is not necessary for that purpose, but drake/Makefile is. Why do you say the top-level Makefile is "for PODS compatibility"?

@jhoare
Copy link

jhoare commented Jul 14, 2016

My mistake, ignore my previous comment, don't remove drake/Makefile. Sorry everyone!

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@david-german-tri
Copy link
Contributor

@jhoare thanks for clarifying! My question to @mwoehlke-kitware still applies, though.

@jhoare
Copy link

jhoare commented Jul 14, 2016

@david-german-tri the root level Makefile was the pod-build entry point into building the superbuild with pods. I believe thats what @mwoehlke-kitware is referring to.

Along the line of de-podsing the superbuild, does addpath_pods.m still have a purpose in life or should it be removed along with this Makefile? The comment here: #2768 (comment) says it should not be used, so if true, should be removed with the same motivation as removing the superbuild pods Makefile.

@mwoehlke-kitware
Copy link
Contributor Author

The top-level Makefile allows the Drake superbuild to be built "as a POD". The drake/Makefile allows Drake itself to be built "as a POD". The comments in #2484 suggest that folks care about the latter, but not as much the former.

@david-german-tri
Copy link
Contributor

OK, thanks. @RussTedrake, @wxmerkt says that OpenHumanoids has no need of superbuild-as-POD. Are there any other consumers we should worry about?

@jwnimmer-tri
Copy link
Collaborator

If we decide to go ahead with this, it definitely needs a CHANGELOG.md entry.

@mwoehlke-kitware
Copy link
Contributor Author

Is anyone maintaining (i.e. in an independent repository) a generic Makefile for wrapping a CMake project "as a POD"? It might be nice to mention that in a changelog entry.

@david-german-tri
Copy link
Contributor

Are there any other consumers we should worry about?

I got a verbal "no" from @RussTedrake, so I think there's nothing blocking this, but I'll leave it to Russ to LGTM and merge.

@mwoehlke-kitware mwoehlke-kitware mentioned this pull request Jul 15, 2016
Merged
@david-german-tri
Copy link
Contributor

david-german-tri commented Jul 15, 2016 •
edited
Loading

Is anyone maintaining (i.e. in an independent repository) a generic Makefile for wrapping a CMake project "as a POD"? It might be nice to mention that in a changelog entry.

There's the PODs repository on sourceforge. Maybe @ashuang can tell us if that's the right thing to cite?

@RussTedrake
Copy link
Contributor

RussTedrake commented Jul 15, 2016 via email

@RussTedrake
Copy link
Contributor

apart from needing the changelog entry, it looks good to me.

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@RussTedrake
Copy link
Contributor

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

Comments from Reviewable

@mwoehlke-kitware
Note removal of top-level Makefile
a085390 
Note the removal of the PODS compatibility Makefile interface to the
drake superbuild in the changelog, and note a possible replacement for
users requiring this functionality.
@mwoehlke-kitware
Copy link
Contributor Author

There's the PODs repository on sourceforge.

Our Makefiles started from those but have matured considerably at this point.

Maybe we should update upstream, then?

@RussTedrake
Copy link
Contributor

nobody has touched the PODS website in years and years.

On Mon, Jul 18, 2016 at 9:43 AM, Matthew Woehlke [email protected]
wrote:

There's the PODs repository on sourceforge
https://sourceforge.net/p/pods/svn/HEAD/tree/.

Our Makefiles started from those but have matured considerably at this
point.

Maybe we should update upstream, then?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2809 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGJNNFsCp-EgYS1DT0_hNFQhHQmoYjrFks5qW4L_gaJpZM4JMyLG
.

@david-german-tri
Copy link
Contributor

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

Comments from Reviewable

@david-german-tri
Copy link
Contributor

:lgtm:

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

Comments from Reviewable

@david-german-tri david-german-tri merged commit 08c4ca3 into RobotLocomotion:master Jul 18, 2016
@ashuang
Copy link

ashuang commented Jul 18, 2016

Hi all,

Yes, I wouldn't worry about pods upstream. For context, that was something that Abe Bacharch and I did a while ago to make it easier for folks (students, research groups, etc.) to more easily exchange software modules, and I think it succeeded in that respect. If you're building a large complex repo there are probably a hundred better ways to do your build systems. It's also not something that we maintain or really even support anymore, as both of us work on different types of software now.

@mwoehlke-kitware mwoehlke-kitware deleted the no-pods-makefile branch July 18, 2016 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@RussTedrake RussTedrake

@david-german-tri david-german-tri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mwoehlke-kitware @jhoare @liangfok @david-german-tri @jwnimmer-tri @RussTedrake @ashuang