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

Support NVCC better? #797

Closed
eddelbuettel opened this Issue Jan 9, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@eddelbuettel
Member

eddelbuettel commented Jan 9, 2018

Cf email discussion on rcpp-devel in thread titled "NVCC compatibility" started by @cdeterman.

One issue, apparently found first by @jtilly and noted here in his RcppThrust README is that this line need to drop const. Can we just add a non-const version too?

(Typo fixed. Added New Year's Resolution to only copy addresses and never ever type free hand, cf fortunes::fortune("dirk can type"))

@Enchufa2

This comment has been minimized.

Contributor

Enchufa2 commented Jan 9, 2018

Quick comment to ping @jtilly (typo above) and to note that this getter is the only one in that file not qualified as const, which could solve the problem without discarding the other one.

@Enchufa2

This comment has been minimized.

Contributor

Enchufa2 commented Jan 9, 2018

Oh, and consequently, this operator should be qualified as const too.

@cdeterman

This comment has been minimized.

cdeterman commented Jan 9, 2018

@Enchufa2 I have tried modifying that getter and operator both as const with my own gpuRcuda but the following error is still returned

/usr/local/lib/R/site-library/Rcpp/include/Rcpp/vector/proxy.h(99): error: the object has type qualifiers that are not compatible with the member function "Rcpp::internal::string_name_proxy::get"
object type is: const Rcpp::internal::string_name_proxy

@Enchufa2

This comment has been minimized.

Contributor

Enchufa2 commented Jan 9, 2018

@cdeterman Have you cleaned up before rebuilding and reinstalled correctly? Because I've tried with docker and your package compiles just fine (with warnings, but without error). This is the diff. I can create a pull request if @eddelbuettel agrees.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Jan 9, 2018

Narrow-ish change to one setter / adding one getter as discussed sounds fine to me.

Diff looks good. If you are familiar enough with the format an entry in ChangeLog is always appreciated too.

@Enchufa2

This comment has been minimized.

Contributor

Enchufa2 commented Jan 9, 2018

Fine. I'll prepare a Dockerfile too and append it to the PR, so that you can test it with one command.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Jan 9, 2018

Why would I need a Dockerfile? I tend to just rebuild Rcpp and then use my fancy new parallel rev dep runner to test'em.

@Enchufa2

This comment has been minimized.

Contributor

Enchufa2 commented Jan 9, 2018

I mean, to test that nvcc does not complain anymore.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Jan 9, 2018

Ah, yes. I had created an nvidia-enabled container the other day (see here) but I have my docker binary and nvidia-docker out of sync.

But nvcc is @cdeterman 's problem, I just want to ensure we don't break Rcpp for all :)

@cdeterman

This comment has been minimized.

cdeterman commented Jan 10, 2018

@Enchufa2 you are correct, I overlooked cleaning up my previous build. Sorry for confusion. The changes in the provided commit indeed do address the error. Thanks for all your help everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment