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

fix/opencl-framework fixes building for OSX #52

Merged
merged 1 commit into from
Mar 7, 2016

Conversation

johnnyman727
Copy link
Contributor

OSX Requires a -framework option to compile opencl

FIX #46

@GitCop
Copy link

GitCop commented Mar 4, 2016

There were the following issues with your Pull Request

  • Commit: 12bc044
    • Commits must be in the following format: %{type}/%{scope}: %{description}

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

@johnnyman727 johnnyman727 mentioned this pull request Mar 4, 2016
OSX Requires a -framework option to compile opencl

FIX autumnai#46
@hobofan
Copy link
Member

hobofan commented Mar 5, 2016

Is that change everything that is necessary to get Collenchyma to compile on OS X?
From #46 (comment) 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
Copy link
Contributor Author

@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
Copy link
Member

hobofan commented Mar 7, 2016

@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
Copy link
Member

hobofan commented Mar 7, 2016

@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 ! 👍

@hobofan
Copy link
Member

hobofan commented Mar 7, 2016

@homu r+

@homu
Copy link
Collaborator

homu commented Mar 7, 2016

📌 Commit 3792b70 has been approved by hobofan

@homu
Copy link
Collaborator

homu commented Mar 7, 2016

⚡ Test exempted - status

@homu homu merged commit 3792b70 into autumnai:master Mar 7, 2016
homu added a commit that referenced this pull request Mar 7, 2016
fix/opencl-framework fixes building for OSX

OSX Requires a -framework option to compile opencl

FIX #46
@hobofan hobofan mentioned this pull request Mar 7, 2016
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.

5 participants