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

chore/osx: support building on os x and test this on travis #59

Merged
merged 1 commit into from
Apr 10, 2016

Conversation

reem
Copy link
Contributor

@reem reem commented Apr 1, 2016

The newly-added build script adds the default installation location of cuda to
rustc's library search path.

Fixes #46

@reem
Copy link
Contributor Author

reem commented Apr 1, 2016

I actually don't think this fixes any issues re: linking OpenCl, but permanently fixing/testing that is blocked on #16.

@reem
Copy link
Contributor Author

reem commented Apr 1, 2016

Just to expand a little: the travis configuration actually does not test with either the opencl or cuda features, because of #16 and because travis doesn't have nvidia GPU nodes respectively, but this fix works on my local machine (OS X 11) with a standard cuda install, and travis now just tests that collenchyma compiles on os x.

@MichaelHirn MichaelHirn mentioned this pull request Apr 3, 2016
@MichaelHirn
Copy link
Member

As already mentioned in #46 at this comment I would like to extend the PR a bit more and introduce already the basic design of the proposed solution in #46. I would make a PR to your branch if you are up to that?

That's correct, that we can't really test runtime behaviour with Travis for GPUs (CUDA, OpenCL) and because of the Travis bug, as described in more detail at #16, also no OpenCL behaviour for now. And there seems to be not CI service who provides GPU support, yet. But if we can test compilement on multiple platforms, that is already incredibly helpful.

Thanks for the .travis.yml 👍

@reem
Copy link
Contributor Author

reem commented Apr 3, 2016

I would definitely take a PR! Unfortunately it looks like the nvidia cuda installers don't support installing to a specific directory, meaning it appears our only option would be to install cuda globally on the users system if it is not already there.

Personally I don't think that's a very appealing route (I'd be quite surprised if cargo install uses-leaf would globally install software), and it would be better to just detect the lack of cuda (or other libraries) and bail out with an error message including a link to instructions on how to get up and running (on os x this is as easy as brew cask install cuda). This is effectively the approach taken by other links-to-C crates like rust-openssl.

As a workaround for the lack of GPU support on travis, we could set up an aws account, include the credentials as encrypted env vars, and run a cuda test on an aws gpu node. This would allow continuous testing of cuda (at least on linux, I don't think you can run os x on an aws gpu node) at the cost of a steep increase in CI setup complexity.

@reem
Copy link
Contributor Author

reem commented Apr 3, 2016

I've updated this PR to only include the changes to the travis configuration, we can deal with adding cuda and opencl support/continuous testing in subsequent PRs.

@MichaelHirn
Copy link
Member

Agreed, installing CUDA globally is something that we should not do when building Collenchyma. A few days ago, we received from NVIDIA direct download links for their cuDNN library, so people can use collenchyma-nn (Leaf on NVIDIA GPUs) more easily. I assume they would have that for CUDA as well. I will ask him.

But for now, I will merge this PR and we introduce the build support in a subsequent one. When you alter the commit message to reflect the new code changes, I think we are ready to merge.

@reem
Copy link
Contributor Author

reem commented Apr 3, 2016

What would you want to change in the commit message? Not fix #46? I think we should close #46 and open new issues for support/testing of opencl and cuda on os x respectively, but I can also just remove the fixes piece and leave it open.

@MichaelHirn
Copy link
Member

I assumed you are altering the commit (removing the build.rs file again) as to just introduce OS X support on Travis with this PR because of

I've updated this PR to only include the changes to the Travis configuration, we can deal with adding cuda and opencl support/continuous testing in subsequent PRs

If we do that, then I would propose to adapt the commit message from

chore/osx: support building on os x and test this on travis

Fixes: #46

to something like

chore/osx: add os x support on travis

I apologize for being not clear enough previously.

Then regarding the #46 issue, I am would do it like you proposed.

Maybe just opening one issue, which bundles the build fixes for opencl/cuda and the various platforms and links to the #46. As soon as we have the osx platform supported we can close #46.

@MichaelHirn
Copy link
Member

Great. Let's merge.

@MichaelHirn
Copy link
Member

@homu r+

@homu
Copy link
Collaborator

homu commented Apr 10, 2016

📌 Commit 2ac7e87 has been approved by MichaelHirn

@homu
Copy link
Collaborator

homu commented Apr 10, 2016

⚡ Test exempted - status

@homu homu merged commit 2ac7e87 into autumnai:master Apr 10, 2016
homu added a commit that referenced this pull request Apr 10, 2016
chore/osx: support building on os x and test this on travis

The newly-added build script adds the default installation location of cuda to
rustc's library search path.

Fixes #46
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.

3 participants