autumnai / collenchyma

Extendable HPC-Framework for CUDA, OpenCL and common CPU
http://autumnai.github.io/collenchyma
Apache License 2.0
475 stars 33 forks source link

fix/opencl-framework fixes building for OSX #52

Closed johnnyman727 closed 8 years ago

johnnyman727 commented 8 years ago

OSX Requires a -framework option to compile opencl

FIX #46

GitCop commented 8 years ago

There were the following issues with your Pull Request

Guidelines are available at https://github.com/autumnai/leaf/blob/develop/CONTRIBUTING.md#git-commit-guidelines


This message was auto-generated by https://gitcop.com

hobofan commented 8 years ago

Is that change everything that is necessary to get Collenchyma to compile on OS X? From https://github.com/autumnai/collenchyma/issues/46#issuecomment-187084973 I got the impression that it also requires a build.rs?

I notice that you didn't mention a build.rs in the gist you posted in gitter. If you can confirm that it works without one, I'll accept the PR, otherwise I'd rather wait for a solution that has full OS X support.

johnnyman727 commented 8 years ago

@hobofan it's certainly possible that the workaround mentioned in #46 is still required. You'll notice in my Gitter that I basically removed compilation of CUDA from the Cargo.toml (because I have an Intel GPU on my macbook air) so I wouldn't have encountered the problems for which @y-ich built those workarounds.

So to get collenchyma compiling out of the box on OSX we need:

  1. Some way to make CUDA compilation optional (targets OSX without NVidia GPUs)
  2. A way to search common CUDA install paths so it can be linked properly (targets OSX with NVidia GPUs)
hobofan commented 8 years ago

@johnnyman727 1. is already possible via feature flags, but we should probably advertise that in the README. (#36, we already have it here in the Leaf README) 2. I am not 100% sure how it works on OS X, but if CUDA is in the default install path it should be detected by something like ld and linked correctly. I am against hardcoding any common CUDA install paths, since they could be very different depending on version of CUDA and OS X. There, some configuration via a build.rs should be better as used in rust-cudnn. I can't really guarantee that that way of configuring works perfectly. I think there are some issues with it if the library is used by another library (so it looks like it works when building from the rust-cudnn project dir, but when used in collenchyma-nn it might not correctly pass all parameters.

hobofan commented 8 years ago

@johnnyman727 That being said, I think I'll accept the PR. Maybe that code fix and enough, but I'll reopen #46, since it isn't guaranteed to be resolved.

Thanks @y-ich and @johnnyman727 ! :+1:

hobofan commented 8 years ago

@homu r+

homu commented 8 years ago

:pushpin: Commit 3792b70 has been approved by hobofan

homu commented 8 years ago

:zap: Test exempted - status