search

RcppCore / Rcpp

Seamless R and C++ Integration
https://www.rcpp.org
GNU General Public License v2.0
741 stars 212 forks source link

potential registration issue when using `_` prefix? #733

Closed kevinushey closed 7 years ago

kevinushey commented 7 years ago

From @krlmlr:

I'm getting bogus errors that seem to relate to the new registration code. I can't replicate them locally yet, but on Travis CI I'm getting test failures iff I use the all-new registration code (with the underscore prefix). I'd like to suggest to hold the release until we get a better understanding of the problem.

It's complicated because the failures occur in the DBItest repo which (effectively) installs the RSQLite repo from source. They don't occur inside the RSQLite repo. See [1] for a failing and [2] for a successful build of the same repo, the only difference is that the second build happened with the old Rcpp registration code [3]. In the failing build, the exported symbol declared last (_RSQLite_init_logging) cannot be loaded from the .so file. Does this ring a bell?

-Kirill

[1] https://travis-ci.org/rstats-db/DBItest/builds/251777532#L3897

[2] https://travis-ci.org/rstats-db/DBItest/builds/251929259

[3] https://github.com/rstats-db/RSQLite/commit/66070b4e

The relevant bit of the error logs, from what I can see:

Error in dyn.load(dllfile) : 
  unable to load shared object '/home/travis/build/rstats-db/DBItest/revdep-dev/RSQLite/src/RSQLite.so':
  /home/travis/build/rstats-db/DBItest/revdep-dev/RSQLite/src/RSQLite.so: undefined symbol: _RSQLite_init_logging
Calls: <Anonymous> -> load_dll -> library.dynam2 -> dyn.load

The only thing I can divine is that, normally, C-style name mangling will prepend an underscore to the symbol name, so we might expect the symbol to actually be called __RSQLite_init_logging. Indeed that's what I see on a local installation:

$ nm RSQLite.so  | grep RSQLite
000000000003f360 T _R_init_RSQLite
000000000003e7d0 T __RSQLite_rsqliteVersion
000000000003c0a0 T __RSQLite_rsqlite_bind_rows
000000000003acb0 T __RSQLite_rsqlite_clear_result
000000000003db00 T __RSQLite_rsqlite_column_info
00000000000363b0 T __RSQLite_rsqlite_connect
0000000000039700 T __RSQLite_rsqlite_connection_valid
0000000000039080 T __RSQLite_rsqlite_copy_database
0000000000038a30 T __RSQLite_rsqlite_disconnect
000000000003b300 T __RSQLite_rsqlite_fetch
000000000003ba20 T __RSQLite_rsqlite_get_placeholder_names
000000000003c750 T __RSQLite_rsqlite_has_completed
0000000000039d90 T __RSQLite_rsqlite_import_file
000000000003eda0 T __RSQLite_rsqlite_init_logging
000000000003e150 T __RSQLite_rsqlite_result_valid
000000000003cdd0 T __RSQLite_rsqlite_row_count
000000000003d480 T __RSQLite_rsqlite_rows_affected
000000000003a5d0 T __RSQLite_rsqlite_send_query

so it's possible this is something specific to the Travis environment in which @krlmlr was testing. (Note that the package builds, installs, and loads fine for me locally)

eddelbuettel commented 7 years ago

How close could we get by working in a Docker container starting from the same Ubuntu releases?

krlmlr commented 7 years ago

I've been unable to replicate with a clean library. Will look into replicating on Docker.

krlmlr commented 7 years ago

Breaks for me on Docker:

FROM rocker/tidyverse

RUN R -q -e 'install.packages(c("devtools", "roxygen2", "testthat"))'

RUN git clone https://github.com/rstats-db/DBItest.git

RUN R -q -e 'devtools::install_deps("DBItest", dep = TRUE)'

# Omit this line to fix the build
RUN cd DBItest && git checkout 2080f7a2

RUN make --directory DBItest/revdep-dev

I understand that the problem is likely buried between multiple obscure layers, happy to do more debugging. Perhaps we can also learn from the generated DBItest/revdep-dev/RSQLite/src/RSQLite.so file, RSQLite.so.gz.

eddelbuettel commented 7 years ago

You missed the part where I said "starting from the same Ubuntu releases". Travis does not use tidyverse as a start container.

Moreover, Rocker container use Debian. Travis is based on Ubuntu. I'd call this helpful but anecdotal.

What I would do is docker run --rm -ti -v $(pwd):/mnt ubuntu:14.04 (or 12:04 ?) started in your ~/git (or whereever) so that you have access to your sources. The just hit apt-get install til you have your build depends.

Closer to Travis CI in my eyes.

krlmlr commented 7 years ago

I can see the same symptoms with my Docker recipe that I see on Travis. Identical symptoms. The recipe has the advantage of being self-contained. Should it turn out that resolving the problems with my Docker recipe doesn't resolve them on Travis (which I find unlikely), we might get an even better understanding of the underlying problem.

I'll work on double-checking with my Docker recipe that this is indeed a regression in Rcpp. Let's hope it affects only my workflow.

kevinushey commented 7 years ago

It looks like something weird is going on with the RcppExports.cpp and RcppExports.R files in the revdep version of the RSQLite package being built. I see e.g.

static const R_CallMethodDef CallEntries[] = {
    {"RSQLite_rsqlite_connect", (DL_FUNC) &RSQLite_rsqlite_connect, 4},
    {"RSQLite_rsqlite_disconnect", (DL_FUNC) &RSQLite_rsqlite_disconnect, 1},
    {"RSQLite_rsqlite_copy_database", (DL_FUNC) &RSQLite_rsqlite_copy_database, 2},
    {"RSQLite_rsqlite_connection_valid", (DL_FUNC) &RSQLite_rsqlite_connection_valid, 1},
    {"RSQLite_rsqlite_import_file", (DL_FUNC) &RSQLite_rsqlite_import_file, 6},
    {"RSQLite_rsqlite_send_query", (DL_FUNC) &RSQLite_rsqlite_send_query, 2},
    {"RSQLite_rsqlite_clear_result", (DL_FUNC) &RSQLite_rsqlite_clear_result, 1},
    {"RSQLite_rsqlite_fetch", (DL_FUNC) &RSQLite_rsqlite_fetch, 2},
    {"RSQLite_rsqlite_get_placeholder_names", (DL_FUNC) &RSQLite_rsqlite_get_placeholder_names, 1},
    {"RSQLite_rsqlite_bind_rows", (DL_FUNC) &RSQLite_rsqlite_bind_rows, 2},
    {"RSQLite_rsqlite_has_completed", (DL_FUNC) &RSQLite_rsqlite_has_completed, 1},
    {"RSQLite_rsqlite_row_count", (DL_FUNC) &RSQLite_rsqlite_row_count, 1},
    {"RSQLite_rsqlite_rows_affected", (DL_FUNC) &RSQLite_rsqlite_rows_affected, 1},
    {"RSQLite_rsqlite_column_info", (DL_FUNC) &RSQLite_rsqlite_column_info, 1},
    {"RSQLite_rsqlite_result_valid", (DL_FUNC) &RSQLite_rsqlite_result_valid, 1},
    {"RSQLite_rsqliteVersion", (DL_FUNC) &RSQLite_rsqliteVersion, 0},
    {"RSQLite_rsqlite_init_logging", (DL_FUNC) &RSQLite_rsqlite_init_logging, 1},
    {"_RSQLite_rsqlite_bind_rows",             (DL_FUNC) &_RSQLite_rsqlite_bind_rows,             2},
    {"_RSQLite_rsqlite_clear_result",          (DL_FUNC) &_RSQLite_rsqlite_clear_result,          1},
    {"_RSQLite_rsqlite_column_info",           (DL_FUNC) &_RSQLite_rsqlite_column_info,           1},
    {"_RSQLite_rsqlite_connect",               (DL_FUNC) &_RSQLite_rsqlite_connect,               4},
    {"_RSQLite_rsqlite_connection_valid",      (DL_FUNC) &_RSQLite_rsqlite_connection_valid,      1},
    {"_RSQLite_rsqlite_copy_database",         (DL_FUNC) &_RSQLite_rsqlite_copy_database,         2},
    {"_RSQLite_rsqlite_disconnect",            (DL_FUNC) &_RSQLite_rsqlite_disconnect,            1},
    {"_RSQLite_rsqlite_fetch",                 (DL_FUNC) &_RSQLite_rsqlite_fetch,                 2},
    {"_RSQLite_rsqlite_get_placeholder_names", (DL_FUNC) &_RSQLite_rsqlite_get_placeholder_names, 1},
    {"_RSQLite_rsqlite_has_completed",         (DL_FUNC) &_RSQLite_rsqlite_has_completed,         1},
    {"_RSQLite_rsqlite_import_file",           (DL_FUNC) &_RSQLite_rsqlite_import_file,           6},
    {"_RSQLite_rsqlite_init_logging",          (DL_FUNC) &_RSQLite_rsqlite_init_logging,          1},
    {"_RSQLite_rsqlite_result_valid",          (DL_FUNC) &_RSQLite_rsqlite_result_valid,          1},
    {"_RSQLite_rsqlite_row_count",             (DL_FUNC) &_RSQLite_rsqlite_row_count,             1},
    {"_RSQLite_rsqlite_rows_affected",         (DL_FUNC) &_RSQLite_rsqlite_rows_affected,         1},
    {"_RSQLite_rsqlite_send_query",            (DL_FUNC) &_RSQLite_rsqlite_send_query,            2},
    {"_RSQLite_rsqliteVersion",                (DL_FUNC) &_RSQLite_rsqliteVersion,                0},
    {NULL, NULL, 0}
};

whereas in a standalone build I see:

static const R_CallMethodDef CallEntries[] = {
    {"_RSQLite_rsqlite_connect", (DL_FUNC) &_RSQLite_rsqlite_connect, 4},
    {"_RSQLite_rsqlite_disconnect", (DL_FUNC) &_RSQLite_rsqlite_disconnect, 1},
    {"_RSQLite_rsqlite_copy_database", (DL_FUNC) &_RSQLite_rsqlite_copy_database, 2},
    {"_RSQLite_rsqlite_connection_valid", (DL_FUNC) &_RSQLite_rsqlite_connection_valid, 1},
    {"_RSQLite_rsqlite_import_file", (DL_FUNC) &_RSQLite_rsqlite_import_file, 6},
    {"_RSQLite_rsqlite_send_query", (DL_FUNC) &_RSQLite_rsqlite_send_query, 2},
    {"_RSQLite_rsqlite_clear_result", (DL_FUNC) &_RSQLite_rsqlite_clear_result, 1},
    {"_RSQLite_rsqlite_fetch", (DL_FUNC) &_RSQLite_rsqlite_fetch, 2},
    {"_RSQLite_rsqlite_get_placeholder_names", (DL_FUNC) &_RSQLite_rsqlite_get_placeholder_names, 1},
    {"_RSQLite_rsqlite_bind_rows", (DL_FUNC) &_RSQLite_rsqlite_bind_rows, 2},
    {"_RSQLite_rsqlite_has_completed", (DL_FUNC) &_RSQLite_rsqlite_has_completed, 1},
    {"_RSQLite_rsqlite_row_count", (DL_FUNC) &_RSQLite_rsqlite_row_count, 1},
    {"_RSQLite_rsqlite_rows_affected", (DL_FUNC) &_RSQLite_rsqlite_rows_affected, 1},
    {"_RSQLite_rsqlite_column_info", (DL_FUNC) &_RSQLite_rsqlite_column_info, 1},
    {"_RSQLite_rsqlite_result_valid", (DL_FUNC) &_RSQLite_rsqlite_result_valid, 1},
    {"_RSQLite_rsqliteVersion", (DL_FUNC) &_RSQLite_rsqliteVersion, 0},
    {"_RSQLite_rsqlite_init_logging", (DL_FUNC) &_RSQLite_rsqlite_init_logging, 1},
    {NULL, NULL, 0}
};

It looks like a bunch of entries got duplicated in the revdep version of the package, but it's not clear to me why this occurred. Is it possible that Rcpp::compileAttributes() is being called with an older / out of date version of Rcpp? Or are you calling tools::package_native_routine_registration_skeleton() somewhere else?

kevinushey commented 7 years ago

Okay, I think I know what the issue here is. Here's the likely series of events that got you into that state:

  1. Install development version of Rcpp,
  2. Call Rcpp::compileAttributes(),
  3. Install the CRAN version of Rcpp,
  4. Call Rcpp::compileAttributes().

If that occurs, I think Rcpp gets confused and ends up generating those duplicate symbol definitions.

I'm guessing your particular case would be resolved by ensuring that the development version of Rcpp is installed + used during revdep testing there.

kevinushey commented 7 years ago

It looks like there might be some work on the Rcpp side required nonetheless, as these steps lead to a package with duplicated symbol entries in src/RcppExports.cpp:

restart <- getOption("restart")

unlink("R/RcppExports.R")
unlink("src/RcppExports.cpp")

install.packages("Rcpp")
restart()

Rcpp::compileAttributes()
restart()

devtools::install_github("RcppCore/Rcpp")
restart()

Rcpp::compileAttributes()
restart()

If I follow that sequence of events, I get a CallEntries[] array of form:

static const R_CallMethodDef CallEntries[] = {
    {"_RSQLite_rsqlite_connect", (DL_FUNC) &_RSQLite_rsqlite_connect, 4},
    {"_RSQLite_rsqlite_disconnect", (DL_FUNC) &_RSQLite_rsqlite_disconnect, 1},
    {"_RSQLite_rsqlite_copy_database", (DL_FUNC) &_RSQLite_rsqlite_copy_database, 2},
    {"_RSQLite_rsqlite_connection_valid", (DL_FUNC) &_RSQLite_rsqlite_connection_valid, 1},
    {"_RSQLite_rsqlite_import_file", (DL_FUNC) &_RSQLite_rsqlite_import_file, 6},
    {"_RSQLite_rsqlite_send_query", (DL_FUNC) &_RSQLite_rsqlite_send_query, 2},
    {"_RSQLite_rsqlite_clear_result", (DL_FUNC) &_RSQLite_rsqlite_clear_result, 1},
    {"_RSQLite_rsqlite_fetch", (DL_FUNC) &_RSQLite_rsqlite_fetch, 2},
    {"_RSQLite_rsqlite_get_placeholder_names", (DL_FUNC) &_RSQLite_rsqlite_get_placeholder_names, 1},
    {"_RSQLite_rsqlite_bind_rows", (DL_FUNC) &_RSQLite_rsqlite_bind_rows, 2},
    {"_RSQLite_rsqlite_has_completed", (DL_FUNC) &_RSQLite_rsqlite_has_completed, 1},
    {"_RSQLite_rsqlite_row_count", (DL_FUNC) &_RSQLite_rsqlite_row_count, 1},
    {"_RSQLite_rsqlite_rows_affected", (DL_FUNC) &_RSQLite_rsqlite_rows_affected, 1},
    {"_RSQLite_rsqlite_column_info", (DL_FUNC) &_RSQLite_rsqlite_column_info, 1},
    {"_RSQLite_rsqlite_result_valid", (DL_FUNC) &_RSQLite_rsqlite_result_valid, 1},
    {"_RSQLite_rsqliteVersion", (DL_FUNC) &_RSQLite_rsqliteVersion, 0},
    {"_RSQLite_rsqlite_init_logging", (DL_FUNC) &_RSQLite_rsqlite_init_logging, 1},
    {"RSQLite_rsqlite_bind_rows",             (DL_FUNC) &RSQLite_rsqlite_bind_rows,             2},
    {"RSQLite_rsqlite_clear_result",          (DL_FUNC) &RSQLite_rsqlite_clear_result,          1},
    {"RSQLite_rsqlite_column_info",           (DL_FUNC) &RSQLite_rsqlite_column_info,           1},
    {"RSQLite_rsqlite_connect",               (DL_FUNC) &RSQLite_rsqlite_connect,               4},
    {"RSQLite_rsqlite_connection_valid",      (DL_FUNC) &RSQLite_rsqlite_connection_valid,      1},
    {"RSQLite_rsqlite_copy_database",         (DL_FUNC) &RSQLite_rsqlite_copy_database,         2},
    {"RSQLite_rsqlite_disconnect",            (DL_FUNC) &RSQLite_rsqlite_disconnect,            1},
    {"RSQLite_rsqlite_fetch",                 (DL_FUNC) &RSQLite_rsqlite_fetch,                 2},
    {"RSQLite_rsqlite_get_placeholder_names", (DL_FUNC) &RSQLite_rsqlite_get_placeholder_names, 1},
    {"RSQLite_rsqlite_has_completed",         (DL_FUNC) &RSQLite_rsqlite_has_completed,         1},
    {"RSQLite_rsqlite_import_file",           (DL_FUNC) &RSQLite_rsqlite_import_file,           6},
    {"RSQLite_rsqlite_init_logging",          (DL_FUNC) &RSQLite_rsqlite_init_logging,          1},
    {"RSQLite_rsqlite_result_valid",          (DL_FUNC) &RSQLite_rsqlite_result_valid,          1},
    {"RSQLite_rsqlite_row_count",             (DL_FUNC) &RSQLite_rsqlite_row_count,             1},
    {"RSQLite_rsqlite_rows_affected",         (DL_FUNC) &RSQLite_rsqlite_rows_affected,         1},
    {"RSQLite_rsqlite_send_query",            (DL_FUNC) &RSQLite_rsqlite_send_query,            2},
    {"RSQLite_rsqliteVersion",                (DL_FUNC) &RSQLite_rsqliteVersion,                0},
    {NULL, NULL, 0}
};

In other words, we now have both the old-style and the new-style symbol registration contained within RcppExports.cpp.

Note that this issue goes away if you either:

  1. Remove src/RcppExports.cpp before calling Rcpp::compileAttributes(), or
  2. Call Rcpp::compileAttributes() twice.

Given that the issue is likely to be quite transient for most users (people will often build + reload their package during development) we could probably get away with doing nothing on the Rcpp side.

cc: @jjallaire

eddelbuettel commented 7 years ago

What is restart() ie getOption("restart") ?

kevinushey commented 7 years ago

Sorry; RStudio populates that as a means of restarting the R session within RStudio. (I'm not sure if there's an Emacs / ESS equivalent)

eddelbuettel commented 7 years ago

I surmised it would have to be RStudio or devtools; I'd do all this on the cmdline with littler helpers.

jjallaire commented 7 years ago

Yes, given that it requires that multiple versions of Rcpp be in play and that it goes away with a second call to compileAttributes I think we are better off doing nothing here.

On Mon, Jul 10, 2017 at 6:12 PM, Kevin Ushey notifications@github.com wrote:

It looks like there might be some work on the Rcpp side required nonetheless, as these steps lead to a package with duplicated symbol entries in src/RcppExports.cpp:

restart <- getOption("restart")

unlink("R/RcppExports.R") unlink("src/RcppExports.cpp")

install.packages("Rcpp") restart()

Rcpp::compileAttributes() restart()

devtools::install_github("RcppCore/Rcpp") restart()

Rcpp::compileAttributes() restart()

If I follow that sequence of events, I get a CallEntries[] array of form:

static const R_CallMethodDef CallEntries[] = { {"_RSQLite_rsqlite_connect", (DL_FUNC) &_RSQLite_rsqlite_connect, 4}, {"_RSQLite_rsqlite_disconnect", (DL_FUNC) &_RSQLite_rsqlite_disconnect, 1}, {"_RSQLite_rsqlite_copy_database", (DL_FUNC) &_RSQLite_rsqlite_copy_database, 2}, {"_RSQLite_rsqlite_connection_valid", (DL_FUNC) &_RSQLite_rsqlite_connection_valid, 1}, {"_RSQLite_rsqlite_import_file", (DL_FUNC) &_RSQLite_rsqlite_import_file, 6}, {"_RSQLite_rsqlite_send_query", (DL_FUNC) &_RSQLite_rsqlite_send_query, 2}, {"_RSQLite_rsqlite_clear_result", (DL_FUNC) &_RSQLite_rsqlite_clear_result, 1}, {"_RSQLite_rsqlite_fetch", (DL_FUNC) &_RSQLite_rsqlite_fetch, 2}, {"_RSQLite_rsqlite_get_placeholder_names", (DL_FUNC) &_RSQLite_rsqlite_get_placeholder_names, 1}, {"_RSQLite_rsqlite_bind_rows", (DL_FUNC) &_RSQLite_rsqlite_bind_rows, 2}, {"_RSQLite_rsqlite_has_completed", (DL_FUNC) &_RSQLite_rsqlite_has_completed, 1}, {"_RSQLite_rsqlite_row_count", (DL_FUNC) &_RSQLite_rsqlite_row_count, 1}, {"_RSQLite_rsqlite_rows_affected", (DL_FUNC) &_RSQLite_rsqlite_rows_affected, 1}, {"_RSQLite_rsqlite_column_info", (DL_FUNC) &_RSQLite_rsqlite_column_info, 1}, {"_RSQLite_rsqlite_result_valid", (DL_FUNC) &_RSQLite_rsqlite_result_valid, 1}, {"_RSQLite_rsqliteVersion", (DL_FUNC) &_RSQLite_rsqliteVersion, 0}, {"_RSQLite_rsqlite_init_logging", (DL_FUNC) &_RSQLite_rsqlite_init_logging, 1}, {"RSQLite_rsqlite_bind_rows", (DL_FUNC) &RSQLite_rsqlite_bind_rows, 2}, {"RSQLite_rsqlite_clear_result", (DL_FUNC) &RSQLite_rsqlite_clear_result, 1}, {"RSQLite_rsqlite_column_info", (DL_FUNC) &RSQLite_rsqlite_column_info, 1}, {"RSQLite_rsqlite_connect", (DL_FUNC) &RSQLite_rsqlite_connect, 4}, {"RSQLite_rsqlite_connection_valid", (DL_FUNC) &RSQLite_rsqlite_connection_valid, 1}, {"RSQLite_rsqlite_copy_database", (DL_FUNC) &RSQLite_rsqlite_copy_database, 2}, {"RSQLite_rsqlite_disconnect", (DL_FUNC) &RSQLite_rsqlite_disconnect, 1}, {"RSQLite_rsqlite_fetch", (DL_FUNC) &RSQLite_rsqlite_fetch, 2}, {"RSQLite_rsqlite_get_placeholder_names", (DL_FUNC) &RSQLite_rsqlite_get_placeholder_names, 1}, {"RSQLite_rsqlite_has_completed", (DL_FUNC) &RSQLite_rsqlite_has_completed, 1}, {"RSQLite_rsqlite_import_file", (DL_FUNC) &RSQLite_rsqlite_import_file, 6}, {"RSQLite_rsqlite_init_logging", (DL_FUNC) &RSQLite_rsqlite_init_logging, 1}, {"RSQLite_rsqlite_result_valid", (DL_FUNC) &RSQLite_rsqlite_result_valid, 1}, {"RSQLite_rsqlite_row_count", (DL_FUNC) &RSQLite_rsqlite_row_count, 1}, {"RSQLite_rsqlite_rows_affected", (DL_FUNC) &RSQLite_rsqlite_rows_affected, 1}, {"RSQLite_rsqlite_send_query", (DL_FUNC) &RSQLite_rsqlite_send_query, 2}, {"RSQLite_rsqliteVersion", (DL_FUNC) &RSQLite_rsqliteVersion, 0}, {NULL, NULL, 0} };

In other words, we now have both the old-style and the new-style symbol registration contained within RcppExports.cpp.

Note that this issue goes away if you either:

  1. Remove src/RcppExports.cpp before calling Rcpp::compileAttributes(), or
  2. Call Rcpp::compileAttributes() twice.

Given that the issue is likely to be quite transient for most users (people will often build + reload their package during development) we could probably get away with doing nothing on the Rcpp side.

cc: @jjallaire https://github.com/jjallaire

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RcppCore/Rcpp/issues/733#issuecomment-314264935, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGXx5lqoYn_v-SmaoDnh67r0PRF1bWcks5sMqHbgaJpZM4OTIY2 .

krlmlr commented 7 years ago

@kevinushey: Agree with your analysis, I forgot that load_all() recompiles Rcpp attributes. Also agree to do nothing to resolve this, but the need to (sometimes) run twice should be prominent in the NEWS. (Can we just run the code internal to compileAttributes() twice in the next and perhaps the subsequent CRAN version of Rcpp?)

eddelbuettel commented 7 years ago

Why do you need it twice? In the five or so years we had compileAttributes(), I only have had the sequence of exactly one each of compileAttribute(), roxygenize() (when used) followed by R CMD INSTALL (or check or ...).

kevinushey commented 7 years ago

Running twice is just a bandaid fix for the transition from current-CRAN Rcpp to next-CRAN Rcpp; after that transition occurs and users rebuild their packages a couple of times users presumedly won't ever encounter this issue again.

eddelbuettel commented 7 years ago

I am still not convinced as we have yet to hear from single user not named Kirill that something broke.

Not saying there can't / won't be transitions. Early users have had src/init.c since Feb or March; Rcpp 0.12.11 also helped them now 0.12.12 changes a little.

But I repeat the official statement that if you use R CMD INSTALL ... and friends (as opposed to devtools) then there appears to be no issue. That still stands, no?

jjallaire commented 7 years ago

Agreeing with previous sentiment that we shouldn't do anything here (e.g. running twice for a period of time) as this is a particular combination that is known and can be worked around locally.

On Tue, Jul 11, 2017 at 12:41 PM, Dirk Eddelbuettel < notifications@github.com> wrote:

I am still not convinced as we have yet to hear from single user not named Kirill that something broke.

Not saying there can't / won't be transitions. Early users have had src/init.c since Feb or March; Rcpp 0.12.11 also helped them now 0.12.12 changes a little.

But I repeat the official statement that if you use R CMD INSTALL ... and friends (as opposed to devtools) then there appears to be no issue. That still stands, no?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RcppCore/Rcpp/issues/733#issuecomment-314502793, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGXx2VS4dQx2Cz1U25-HN8dzJQPa_ncks5sM6WsgaJpZM4OTIY2 .

kevinushey commented 7 years ago

For posterity, here's a Bash script that illustrates the issue:

#!/usr/bin/env bash

cd $TMPDIR
rm -rf dummy
R -e 'Rcpp::Rcpp.package.skeleton("dummy")'
cd dummy

rm -f src/RcppExports.cpp
R -e 'options(repos = c(CRAN = "https://cran.rstudio.com")); install.packages("Rcpp")'
R -e 'Rcpp::compileAttributes()'
R -e 'options(repos = c(CRAN = "https://cran.rstudio.com")); devtools::install_github("RcppCore/Rcpp")'
R -e 'Rcpp::compileAttributes()'
cat src/RcppExports.cpp

You could imagine this sequence of events occurring for a user who has updated from CRAN Rcpp to development Rcpp. After doing this, you'll see the following output in RcppExports.cpp:

// Generated by using Rcpp::compileAttributes() -> do not edit by hand
// Generator token: 10BE3573-1514-4C36-9D1C-5A225CD40393

#include <Rcpp.h>

using namespace Rcpp;

// rcpp_hello_world
List rcpp_hello_world();
RcppExport SEXP _dummy_rcpp_hello_world() {
BEGIN_RCPP
    Rcpp::RObject rcpp_result_gen;
    Rcpp::RNGScope rcpp_rngScope_gen;
    rcpp_result_gen = Rcpp::wrap(rcpp_hello_world());
    return rcpp_result_gen;
END_RCPP
}

RcppExport SEXP dummy_rcpp_hello_world();

static const R_CallMethodDef CallEntries[] = {
    {"_dummy_rcpp_hello_world", (DL_FUNC) &_dummy_rcpp_hello_world, 0},
    {"dummy_rcpp_hello_world", (DL_FUNC) &dummy_rcpp_hello_world, 0},
    {NULL, NULL, 0}
};

RcppExport void R_init_dummy(DllInfo *dll) {
    R_registerRoutines(dll, NULL, CallEntries, NULL, NULL);
    R_useDynamicSymbols(dll, FALSE);
}

Note the duplicated symbol registration in the CallEntries array. However, calling Rcpp::compileAttributes() one more time (which most users will do when developing their packages) removes that duplicate entry, hence the bug here will very likely be transient for most users.

achubaty commented 7 years ago

I am still not convinced as we have yet to hear from single user not named Kirill that something broke.

For the record, I had encountered this issue recently too, but as mentioned above, it was transient.

mcol commented 7 years ago

I did too and it was really puzzling, but it was solved by removing RcppExports.cpp and recompiling. My doubt is: would a user of the package I'm writing require Rcpp 0.12.12, or is this backward compatible?

eddelbuettel commented 7 years ago

@mcol You misunderstand what is happening there. We simply write standard source code which is consumed by R itself via R CMD .... and related. There is no particular version dependency: you need a suitable C++ compiler and a suitable R version. There is no requirement for particular Rcpp versions as all we are talking about here are part of the C API of R.

tr8dr commented 7 years ago

I am having the same problem with a package I wrote, where it creates 2 sets of entries for the RccpExport'ed functions. Also I get a bunch of externs for "C' bound functions that don't exist. My exported functions have C++ mangled names, hence the .so shows that the "C" functions are not defined.

...
SEXP cget_indexed(SEXP obj, int ith);
RcppExport SEXP _rDotNet_cget_indexed(SEXP objSEXP, SEXP ithSEXP) {
BEGIN_RCPP
    Rcpp::RObject rcpp_result_gen;
    Rcpp::RNGScope rcpp_rngScope_gen;
    Rcpp::traits::input_parameter< SEXP >::type obj(objSEXP);
    Rcpp::traits::input_parameter< int >::type ith(ithSEXP);
    rcpp_result_gen = Rcpp::wrap(cget_indexed(obj, ith));
    return rcpp_result_gen;
END_RCPP
}
...
RcppExport SEXP rDotNet_ccall(SEXP, SEXP, SEXP);
RcppExport SEXP rDotNet_ccall_static(SEXP, SEXP, SEXP);
RcppExport SEXP rDotNet_cget(SEXP, SEXP);
RcppExport SEXP rDotNet_cget_indexed(SEXP, SEXP);
RcppExport SEXP rDotNet_cinit(SEXP, SEXP);
RcppExport SEXP rDotNet_cnew(SEXP, SEXP);
RcppExport SEXP rDotNet_cset(SEXP, SEXP, SEXP);

static const R_CallMethodDef CallEntries[] = {
    {"_rDotNet_cinit", (DL_FUNC) &_rDotNet_cinit, 2},
    {"_rDotNet_cnew", (DL_FUNC) &_rDotNet_cnew, 2},
    {"_rDotNet_ccall_static", (DL_FUNC) &_rDotNet_ccall_static, 3},
    {"_rDotNet_ccall", (DL_FUNC) &_rDotNet_ccall, 3},
    {"_rDotNet_cget", (DL_FUNC) &_rDotNet_cget, 2},
    {"_rDotNet_cset", (DL_FUNC) &_rDotNet_cset, 3},
    {"_rDotNet_cget_indexed", (DL_FUNC) &_rDotNet_cget_indexed, 2},
    {"rDotNet_ccall",        (DL_FUNC) &rDotNet_ccall,        3},
    {"rDotNet_ccall_static", (DL_FUNC) &rDotNet_ccall_static, 3},
    {"rDotNet_cget",         (DL_FUNC) &rDotNet_cget,         2},
    {"rDotNet_cget_indexed", (DL_FUNC) &rDotNet_cget_indexed, 2},
    {"rDotNet_cinit",        (DL_FUNC) &rDotNet_cinit,        2},
    {"rDotNet_cnew",         (DL_FUNC) &rDotNet_cnew,         2},
    {"rDotNet_cset",         (DL_FUNC) &rDotNet_cset,         3},
    {NULL, NULL, 0}
};

If I manually trim out the forward declared "RcppExport" functions just above the function registration table and remove the function set without "_" in the registry, my package build completes properly. This was working properly in Rcpp 0.12.8 and now fails in 0.12.12. Please advise.

eddelbuettel commented 7 years ago

Run compileAttributes() twice.

tr8dr commented 7 years ago

I am not sure how that is done. I am using the Rcpp recommended approach to creating a package (as far as I know). My NAMESPACE file has:

useDynLib(rDotNet)
exportPattern("^[^\\.]")
export(.cnew, .ccall, .ctor, .cstatic, .cget, .cset, "$.rDotNet", "[.rDotNet", print.rDotNet)
importFrom(Rcpp, evalCpp)

My Makevars file has:

PKG_LIBS = `$(R_HOME)/bin/Rscript -e "Rcpp:::LdFlags()"` -L/usr/local/lib -lboost_system 
PKG_CPPFLAGS = -std=c++0x -I. -I../inst/include 
CXXFLAGS=-std=c++0x -g -O0 -Wall -frob

And my external build deletes the RcppExports.R and RccpExports.cpp files, before running R CMD BATCH UPDATE and then R CMD INSTALL. I have not investigated the magic that occurs with "importFrom(Rcpp, evalCpp)", but assume this runs some preprocessing to generate these files during the UPDATE phase?

eddelbuettel commented 7 years ago

Forcefully deleting RcppExports.R and RcppExports.cpp is unlikely to be have been recommended by us as a method.

compileAttributes() is an R function exported by the Rcpp package. Please call it twice instead. See the 'Rcpp Attributes' vignette for more help if you need it.

I'd try to help but I can't find an rDotNet repo with R code anywhere.

tr8dr commented 7 years ago

My question above related to where / whether this compileAttributes() should be inserted into the package structure? Presumably one would like the package to configure and compile with a single R command. Is it the case then that you are suggesting that I should invoke R to run compileAttributes() from the command line or script?

Is there a longer term fix in mind? As a hack perhaps it would make sense to add the 2 calls to the skeleton avoiding the failures that I am sure others are encountering.

eddelbuettel commented 7 years ago

  1. This thread is not going anywhere, and four messages in you still have not provided anything reproducible.

  2. I happen to call compileAttributes() from the command-line when I need it -- because I happen to like command-line tools. In this case compAttr.r is part of littler. I built those, I also use them.

  3. Many people, myself included, also use RStudio. It calls compileAttributes() for you. As it does for roxygenize() and I have yet to hear someone claim that was unusable.

  4. You seem confused about build processes and possibly caught up in some local tooling you created. If that fails, it is your issue, not ours. Rcpp works just fine using the R CMD ... approach that is both documented and recommended.

tr8dr commented 7 years ago

I appreciate the work you have done with Rcpp. I can run compileAttributes() in R if that is the recommended solution. However as indicated above, I have, what I believe is a standard structure, with a standard-ish NAMESPACE and Makevars. The R CMD UPDATE and R CMD INSTALL are run from the command line (though often as a larger build, but sometimes by hand). What I do know is that nothing has changed in my code or configuration, yet 0.12.8 -> 0.12.12 produces the problem above. So I do not know why you are lashing out. I am hoping to avoid hacks here, but sounds like that is the recommendation.

eddelbuettel commented 7 years ago

I can run compileAttributes() in R

Please do. Twice. The reasons why are explained above.

tr8dr commented 7 years ago

I ran manually twice and still get the declaration of "C" bound functions (which do not exist) and dual naming of "_function" and "function". The dual naming, presumably does not cause any problems, however the extern'ed functions do. The .so indicates that _rDotNet_ccall and 6 other functions are missing. Instead these functions were compiled with C++ mangled names. Rcpp created "C" wrappers for each exported function (i.e. rDotNet_ccall) in the case of the above C++ rDotNet_ccall function. I don't know why Rcpp is trying to export the alternate function naming. I could post the project if it is useful as a test case here. For now I can manually edit the generated file, removing the generated:

RcppExport SEXP rDotNet_ccall(SEXP, SEXP, SEXP);
RcppExport SEXP rDotNet_ccall_static(SEXP, SEXP, SEXP);
RcppExport SEXP rDotNet_cget(SEXP, SEXP);
RcppExport SEXP rDotNet_cget_indexed(SEXP, SEXP);
RcppExport SEXP rDotNet_cinit(SEXP, SEXP);
RcppExport SEXP rDotNet_cnew(SEXP, SEXP);
RcppExport SEXP rDotNet_cset(SEXP, SEXP, SEXP);
eddelbuettel commented 7 years ago
  1. Is there a chance you can provide something reproducible? Is the repo public?
  2. What R version? We did all these changes to Rcpp because R 3.4.* wants it.
tr8dr commented 7 years ago
  1. It is not currently public, but I do not have issue in sharing here. Has a separate .NET component to it and will be eventually released to GitHub. Shall I post the R src here?
  2. This is for R 3.4 (OS X). I just rebuilt my machine, so there are no older versions of R lying around.
eddelbuettel commented 7 years ago

We don't need "it" per se, but we need to see what you did.

So could you do us the favour and create a minimal package exhibiting the same behavior? I doubt .Net enters as R does not see that. It may just be unusually-named functions etc -- but we need to be able to reproduce the behavior if you want us to change it.

From the looks of it this could be a new bug. Or it could be your usage. Impossible for me to tell at this point.

tr8dr commented 7 years ago

I'll try to construct a simple test case on this end ...

tr8dr commented 7 years ago

Ok, creating the Rcpp exports and then building on this test project fails for me (R 3.4 virgin install + Rcpp 0.12.12) on OS X 10.12.6. I have not tried to install the development version from GitHub given that it may confuse the issue. If the latest version does not show a problem, of course am happy to run off dev.

cd rTest
R -e 'Rcpp::compileAttributes()'
R -e 'Rcpp::compileAttributes()'
cd ..
R CMD INSTALL rTest

Will result in a rTest.so with an undefined symbol of _rTest_cnew. Find the tar attached. rTest.tar.gz

eddelbuettel commented 7 years ago

You have two or three issue here which in combination throw you a spanner:

Mixing of manual .Call() and Rcpp Attributes.

Generally a bad idea. I changed your .cnew() to 

## create new object from class
.cnew <- function (classname, ...) {
    argv = list(...)
    ##.Call("rTest_cnew", PACKAGE='rTest', classname, argv)
    cnew(classname, argv)
}

That alone was probably the cause of your issues.

Not using .registration=TRUE in NAMESPACE

Use this instead: useDynLib(rTest, .registration=TRUE)

Missing License: in DESCRIPTION

Aborts R CMD check immediately. Don't do that -- you generally want to run R CMD check regularly.

Redundant Depends + Imports on Rcpp

Does not break anything, but the Depends is not needed anymore when you have Imports.

For completeness, a full R CMD check (again from shell script front end) follows.

/tmp/tr8dr> build.r rTest     # wrapper around R CMD build ...
* checking for file ‘rTest/DESCRIPTION’ ... OK
* preparing ‘rTest’:
* checking DESCRIPTION meta-information ... OK
* cleaning src
* checking for LF line-endings in source and make files
* checking for empty or unneeded directories
Removed empty directory ‘rTest/tests’
* building ‘rTest_1.0.tar.gz’

/tmp/tr8dr> rcc.r rTest_1.0.tar.gz    # wrapper around R CMD check --as-cran
────────────────────────────────────────────────────────────────────────────────
─  using log directory ‘/tmp/file49677eb87675/rTest.Rcheck’
─  using R version 3.4.1 (2017-06-30)
─  using platform: x86_64-pc-linux-gnu (64-bit)
─  using session charset: UTF-8
✔  checking for file ‘rTest/DESCRIPTION’
─  checking extension type ... Package
─  this is package ‘rTest’ version ‘1.0’
✔  checking package namespace information
✔  checking package dependencies
W  checking if this is a source package
   Subdirectory ‘src’ contains:
     CLRApi.hpp RValue.hpp
   These are unlikely file names for src files.
✔  checking if there is a namespace
✔  checking for executable files
✔  checking for hidden files and directories
✔  checking for portable file names
✔  checking for sufficient/correct file permissions
✔  checking whether package ‘rTest’ can be installed
✔  checking installed package size
✔  checking package directory
N  checking DESCRIPTION meta-information
   Malformed Description field: should contain one or more complete sentences.
   Package listed in more than one of Depends, Imports, Suggests, Enhances:
     ‘Rcpp’
   A package should be listed in only one of these fields.
✔  checking top-level files
✔  checking for left-over files
✔  checking index information
✔  checking package subdirectories
✔  checking R files for non-ASCII characters
✔  checking R files for syntax errors
✔  checking whether the package can be loaded
✔  checking whether the package can be loaded with stated dependencies
✔  checking whether the package can be unloaded cleanly
✔  checking whether the namespace can be loaded with stated dependencies
✔  checking whether the namespace can be unloaded cleanly
✔  checking loading without being on the library search path
N  checking dependencies in R code
   Packages in Depends field not imported from:
     ‘BH’ ‘R6’
     These packages need to be imported from (in the NAMESPACE file)
     for when this namespace is loaded but not attached.
✔  checking S3 generic/method consistency
✔  checking replacement functions
✔  checking foreign function calls
N  checking R code for possible problems
   File ‘rTest/R/Init.R’:
     .onLoad has wrong argument list ‘libname, package’

   Package startup functions should have two arguments with names starting
     with ‘lib’ and ‘pkg’, respectively.
   See section ‘Good practice’ in '?.onAttach'.
W  checking for missing documentation entries
   Undocumented code objects:
     ‘_rTest_cinit’ ‘_rTest_cnew’ ‘cinit’ ‘cnew’
   All user-level objects in a package should have documentation entries.
   See chapter ‘Writing R documentation files’ in the ‘Writing R
   Extensions’ manual.
✔  checking line endings in C/C++/Fortran sources/headers
✔  checking line endings in Makefiles
W  checking compilation flags in Makevars
   Non-portable flags in variable 'PKG_CPPFLAGS':
     -std=c++0x
   Variables overriding user/site settings:
     CXXFLAGS: -std=c++0x -g -O0 -Wall -frob
✔  checking for GNU extensions in Makefiles
✔  checking for portable use of $(BLAS_LIBS) and $(LAPACK_LIBS)
✔  checking compiled code
─  checking examples ... NONE
✔  checking PDF version of manual

── 0 errors ✔ | 3 warnings ✖ | 3 notes ✖
/tmp/tr8dr>

I generally recommend trying to get this done to 0 errrors, 0 warnings, 0 notes.

eddelbuettel commented 7 years ago

But in sum: No bug per se in Rcpp.

We simply take less kindly to a mixing of usage patterns but it probably less tolerant of user behavior than it could be but then again ... easy to fix and faster with registered symbols.

I think I am going close this issue with has helped us to get to Rcpp 0.12.12.

tr8dr commented 7 years ago

Ok, The call to cnew(classname, argv), instead of the call to the underlying C symbol made the difference. The other changes were not instrumental in solving this problem. However, given that this is a very low level project, and these functions could be called at high frequency, I wanted to use the .Call to the C function directly, bypassing another layer of R to get to the C side. It would be preferable if I could avoid the extra layer.

I guess in previous versions of Rcpp the R code was not scanned, since this is now only manifesting in 0.12.12. I thought you were using the comment directives in the C / C++ code to determine what to export, but looks as if you are doing analysis of the R side as well.

Finally note that some of these "warnings" are resultant from stripping the project down,

eddelbuettel commented 7 years ago

You continue to shoot the messenger. R 3.4.0 and beyond change and make registration recommended (and undoubtedly at some future point, required). That lead us to change our code.

tr8dr commented 7 years ago

I will add that of course. I was trying to determine what was cause and effect

eddelbuettel commented 7 years ago

And of course you CAN continue to use .Call() directly; we do it too in the generated code. But because of the generated symbols it probably requires changes at your end. Such is life.

I am closing this now.