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

Add basic support for cuda via nvcc toolset #506

Closed
wants to merge 10 commits into from

Conversation

nmoehrle
Copy link
Contributor

@nmoehrle nmoehrle commented Jun 9, 2016

Only gmake has been tested - needs tests

@samsinsane
Copy link
Member

I think you should start off by making this a module, similar to this, work out all the bugs/issues and expand on the supported actions. Additionally, I'm not sure if the scope of Premake-core should include the CUDA compiler, but I guess this depends on how popular your module becomes.

@tvandijck
Copy link
Contributor

this commit seems to contain a lot more then just nvcc at this point..
symbols handling, path normalization, and a bunch or other stuff...

probably best to rebase this against master and get rid of the 'odd' merges.

@tvandijck
Copy link
Contributor

This one fell pretty far through the cracks... I'm so sorry.... Unfortunately at this point there is a merge conflict that requires resolution... other then that the changes look pretty clean and simple, so I'd be OK merging it once that happens.

@nmoehrle
Copy link
Contributor Author

No worries, I have two more commits related to cuda support: nmoehrle/premake-core@3eeb90c8 nmoehrle/premake-core@04daa8e46 but they change the api so I am reluctant to include them into this pull request.

If you want me to squash the commits or add the others, just tell me - happy to help.

@tvandijck
Copy link
Contributor

Those other two commits don't look too off.. feel free to include them in this PR.
I'll squash on merge...

@nmoehrle nmoehrle changed the title Add basic support for cuda via nvcc toolset WIP Add basic support for cuda via nvcc toolset Nov 22, 2016
@samsinsane
Copy link
Member

I still think this should be a module, regardless of whether it's merged in or not. Also, it would be good if this had tests so it doesn't break in the future.

@nmoehrle
Copy link
Contributor Author

I thought it should be next to gcc since it is so tightly coupled (because of nvccs design). Regarding tests I agree... would anyone be willing to give me a little guidance?

@nmoehrle
Copy link
Contributor Author

I looked into the existing tests but I found no sensible way to test the functionality of the nvcc toolset without reimplementing large parts of the gcc, clang and msc test suits. Even if I would reimplement these test suits checking the prefixing I wont detect problems like a6e00c8 where multiple flags are given as a space separated string rather than a list and caused wrong prefixing on new flags.
Writing this I recognize that splitting strings on white space might be a way to prevent this...
I am unsure which parts would make sense to test...

@samsinsane
Copy link
Member

I thought it should be next to gcc since it is so tightly coupled (because of nvccs design).

I can see how you reached that conclusion, but it's more about keeping all of the CUDA changes together. Extending Visual Studio to support CUDA would also fall into this modules/cuda folder. As well as the tests. As an example, look at how the D module handled adding toolsets, extending actions and test cases. The D module is more or less how I expect the CUDA module to end up in the future.

Regarding tests I agree... would anyone be willing to give me a little guidance?

I am unsure which parts would make sense to test...

You'll need to test anything you added, basically. Look at the test cases for GCC, Clang, MSC, SNC, DMD, GDC or LDC. I can see similarities between Clang and nvcc here, Clang is just GCC with some minor changes, the same way that nvcc seems to be GCC, Clang or MSC with minor changes. Also, a key thing that @starkos wants Premake to do is to be able to generate the project files on one OS and send them to another OS. So since you have os.is(...) in your code, you'll need to test explicitly setting the OS (I can guarantee this will fail as the os.is is done on script execution which will probably occur before the OS can be overridden, definitely the case in the test suite).

Let me know if you need anymore help, more than happy to help.

@TurkeyMan
Copy link
Contributor

Incidentally, I also have a CUDA module... if there are any any advantages on either side, they should be merged.
https://github.com/TurkeyMan/premake-cuda

@tvandijck
Copy link
Contributor

Well, this commit is not a module right now.... and it should...
but the feature is a great addition obviously.

@starkos
Copy link
Member

starkos commented Apr 16, 2018

Hey all—it sounds like there is consensus that this should be contributed as a module rather than into core. Is it okay to go ahead and close this PR?

@tvandijck
Copy link
Contributor

I doubt it will get updated to a module by the original author... we can close it, but that will just result in the work getting lost.. making a module out of it would be nice, but that would be on us. Combining it with the cuda module from @TurkeyMan may make sense.

@nmoehrle
Copy link
Contributor Author

nmoehrle commented Apr 17, 2018

Hey guys, sorry that I haven't found the time to convert this into a module and sadly I doubt that I will find it in the near future.

One thing to consider is that since last year clang supports the compilation of cuda which is a viable alternative to nvcc.

tvandijck added a commit to tvandijck/premake-core that referenced this pull request Apr 21, 2018
@tvandijck
Copy link
Contributor

@nmoehrle Hey, could you enable write access to this PR as described here:
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

that way I can spend some time rebasing this against latest, and maybe make a module out of it.

tvandijck added a commit that referenced this pull request Apr 21, 2018
@WorldofBay WorldofBay mentioned this pull request Jan 10, 2019
@samsinsane
Copy link
Member

I'm going to close this off, please feel free to reopen if you get time to work on the branch again.

@samsinsane samsinsane closed this Jan 11, 2019
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