Changes to build R client on Fedora. #4492
Conversation
jcferretti
added
NoDocumentationNeeded
NoReleaseNotesNeeded
jcferretti
requested review from
chipkent,
alexpeters1208 and
rcaudy
as code owners
| @@ -256,7 +257,7 @@ class TableHandleWrapper { | |||
| abs_sort = std::vector<bool>(cols.size(), abs_sort[0]); | |||
| } | |||
|
|
|||
| for(int i = 0; i < cols.size(); i++) { | |||
| for(std::size_t i = 0; i < cols.size(); i++) { | |||
There was a problem hiding this comment.
Is there a reason to include a whole new header file just for this? Is it worth it?
There was a problem hiding this comment.
The type of size() for an std::vector is std::size_t. The old code works but is the wrong way to do it.
So, yes.
| @@ -13,7 +13,7 @@ Description: The `rdeephaven` package provides an R API for communicating with t | |||
| and bind it to a server-side variable so you can access it from any Deephaven client. Finally, you can run Python or Groovy | |||
| scripts on the Deephaven server, so long as your server is equipped with that capability. | |||
| License: Apache License (== 2.0) | |||
| Depends: R (>= 4.1.2), Rcpp (>= 1.0.10), arrow (>= 12.0.0), R6 (>= 2.5.0), dplyr (>= 1.1.0) | |||
| Depends: R (>= 3.5.3), Rcpp (>= 1.0.10), arrow (>= 12.0.0), R6 (>= 2.5.0), dplyr (>= 1.1.0) | |||
There was a problem hiding this comment.
Has this been tested? I assume yes, but I only ask because I seem to recall having devtools issues using such an old version of R.
There was a problem hiding this comment.
I saw that your comment says you ran all the tests, so I guess this is working. I wish I could recall exactly what problems I had with using an old version of R.
There was a problem hiding this comment.
I tested on Fedora 28 which is R 3.5.3, is what the customer that needs enterprise R is using (they use RHEL 8 but that's based on Fedora 28), and is the reason these changes exist in the first place.
No description provided.