-
Notifications
You must be signed in to change notification settings - Fork 79
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: Replace Rcpp by cpp11 #419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. What does the C stack overflow on macOS mean?
DESCRIPTION
Outdated
@@ -48,13 +49,13 @@ LazyLoad: true | |||
Roxygen: list(markdown = TRUE) | |||
RoxygenNote: 7.2.3 | |||
SystemRequirements: libpq >= 9.0: libpq-dev (deb) or postgresql-devel | |||
(rpm) | |||
(rpm), C++17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need C++17?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was warning in CI about default c++17
standard in DESCRIPTION file on the latest Ubuntu as I understood
} | ||
} | ||
|
||
Rcpp::RObject DbColumnStorage::class_from_datatype(DATA_TYPE dt) { | ||
cpp11::sexp DbColumnStorage::class_from_datatype(DATA_TYPE dt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a cpp11 vector object contain R_NilValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cpp11::r_vector
which is used for cpp11::strings
, cpp11::integers
etc., can not be assigned to R_NilValue
, only cpp11::sexp
which in general make sense.
It's seems known problem, I will take a look. The same problem on Mac was with |
use cpp11 and cpp11 only
I've been testing this PR and it works well also, it solves #400 |
Thanks. We need to fix the stack overflow on macOS. |
DESCRIPTION
Outdated
@@ -1,6 +1,6 @@ | |||
Package: RPostgres | |||
Title: Rcpp Interface to PostgreSQL | |||
Version: 1.4.5.9002 | |||
Version: 1.4.5.9003 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't change this in PRs.
DESCRIPTION
Outdated
SystemRequirements: | ||
libpq >= 9.0: libpq-dev (deb) or postgresql-devel (rpm), | ||
C++11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++11 can no longer be mentioned due to recent changes in R-devel. Will the checks succeed if we leave it out?
SystemRequirements: | |
libpq >= 9.0: libpq-dev (deb) or postgresql-devel (rpm), | |
C++11 | |
SystemRequirements: | |
libpq >= 9.0: libpq-dev (deb) or postgresql-devel (rpm) |
@Antonov548 I tested locally and it works. The CI shows a problem with CRAN links, I googled and I found this r-lib/actions#697. Lmk how can I help to fix this. |
@Antonov548 it seems to be ok now r-lib/actions#697 (comment) all the checks here are passing so far, I am using this PR wih my usual data analysis and it works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I have tweaked one test, looks great otherwise.
Some tests now output Error: Disconnected
. I have tracked this down to a failure in DbConnection::check_connection()
when a result set is invalid (?). The commit 373c7a5 succeeds in the main branch, but after merging the main branch here, the tests now fail. Can you please take a look?
@krlmlr Fixed 👍 |
@krlmlr @Antonov548 I pulled the last changes. It works well with Ubuntu 20.04. Here's the full check
|
@krlmlr @Antonov548 I send a PR to fix the notes about bashisms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do one last pass.
Thanks! |
No description provided.