Skip to content
This repository has been archived by the owner on Oct 15, 2019. It is now read-only.

patch for more flexible/idiomatic use of CMake #15

Merged
merged 5 commits into from Mar 23, 2015
Merged

patch for more flexible/idiomatic use of CMake #15

merged 5 commits into from Mar 23, 2015

Conversation

redpony
Copy link
Contributor

@redpony redpony commented Mar 23, 2015

Hi guys,
I've made a few changes that makes the Minerva build slightly easier to link with other applications that are using CMake and the same libraries (e.g., boost). I don't think I've broken anything, and I've tested it on Mac OS without CUDA and Linux with CUDA, but there will be a slightly changed build process when you run ./configure (you'll need to specify the deps/ directory as the CMAKE_PREFIX_PATH, but I've provided instructions). I'm happy to keep my changes separate of course, but I'm submitting this pull request since others might find it useful, and it will help me avoid conflicts going forward if you accept it, so I would be very happy if you incorporated it into your branch.
All the best,
Chris D.

@hotpxl
Copy link
Member

hotpxl commented Mar 23, 2015

Wow. Thanks for making Minerva a better tool. I'm not an expert in CMake. Please allow me @jermainewang to review it.

jermainewang added a commit that referenced this pull request Mar 23, 2015
patch for more flexible/idiomatic use of CMake
@jermainewang jermainewang merged commit a13cd6c into dmlc:master Mar 23, 2015
@jermainewang
Copy link
Member

Hi Chris,

Thank you so much for your patch. Just merged the pull request.
BTW, we are trying to get rid of boost library dependency, so some of your efforts on boost part may be discarded later. Sorry for that.

Thanks again,
Minjie

@jermainewang
Copy link
Member

BTW, Chris, I made some changes on your patch and make it easier to use (hopefully). Currently you don't need to specify -DCMAKE_PREFIX_PATH when using deps directory. Also, there are some other changes including 1. build without debug; 2. build without apps (so lmdb and protobuf is not required); 3. build without unittests (so gtest is not required).

May I ask you to test the new configure and compile settings on your machine? It's ok if you don't have time.

Thank you very much.
Minjie

@redpony
Copy link
Contributor Author

redpony commented Mar 30, 2015

Hi Minjie, sorry I just noticed all these messages- gmail hides github
notifications from me rather badly. I'll keep my eyes open, but I'm more
likely to see these when you copy my work address.

But, thanks for incorporating the change. These were helpful for me to
integrate into the rest of the build system I've got, but everything
continues to build find on my machines with your newest changes. It's very
convenient to just be able to build the tests and no apps.

A couple more things: I've got another patch I'd like to submit- it's
mostly a minor const correctness thing, but i've also got resolve-deps.sh
split into a few pieces (we're working on machines that have strange
versions of some of the Google libraries installed and so pulling down only
the ones we need is helpful). Anyway, it's not a big deal, but the const
correctness point can cause trouble for C++ interface users. Will issue it
shortly.

Glad to hear that you're dropping the dependency on boost. I'll keep that
in mind as we go forward.
-Chris

-Chris

On Mon, Mar 23, 2015 at 6:34 PM, Minjie Wang notifications@github.com
wrote:

BTW, Chris, I made some changes on your patch and make it easier to use
(hopefully). Currently you don't need to specify -DCMAKE_PREFIX_PATH when
using deps directory. Also, there are some other changes including 1.
build without debug; 2. build without apps (so lmdb and protobuf is not
required); 3. build without unittests (so gtest is not required).

May I ask you to test the new configure and compile settings on your
machine? It's ok if you don't have time.

Thank you very much.
Minjie


Reply to this email directly or view it on GitHub
#15 (comment)
.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants