eddelbuettel / rapidatetime

Datetime functionality from the C API for R
GNU General Public License v2.0
11 stars 5 forks source link

Linking to C Routines #4

Closed dcooley closed 10 months ago

dcooley commented 10 months ago

I made a basic package called rdatetime to test how to link to RApiDatetime.

When including #include "RApiDatetime.h" (e.g. here) I get compliation errors:

error: initializer element is not constant
      51 |     static SEXP(*fun)(SEXP,SEXP) = (SEXP(*)(SEXP,SEXP)) R_GetCCallable("RApiDatetime", "asPOSIXlt");
         |      

This is solved by removing static

But this leads to another set or errors

duplicate symbol '_formatPOSIXlt' in:
       init.o
       rdatetime.o

This is then solved by including inline


Should the RApiDatetime.h have these changes - e.g - https://github.com/dcooley/rapidatetime/blob/master/inst/include/RApiDatetime.h

eddelbuettel commented 10 months ago

Hi Dave. Thanks for looking into RApiDateTime.

If memory serves, I wrote for use from anytime -- which no longer uses it. It always offered an alternate there as anytime primarily relies on the Boost Date_time parser. But we saw e.g. (in some early bug reports) some weird TZ issues on your continent to adding a second parser seemed like a good idea, and for a while that was provided by RApiDatetime. It received sufficient scrutiny then but may now have gotten moldy.

With that, your ticket is not yet as clear as it could be:

One issue may be that I may have been a little sloppy in writing the include header so that it currently may only be included in one compilation unit per project (to not get the the multiple symbols). You could dig out older anytime sources to see what it did. The change happened in July 2019 before 0.3.5. Older releases should use RApiDatetime.

eddelbuettel commented 10 months ago

Yeah so you probably do not need to include rdatetime.h in your src/init.c.

As for the error: initializer element is not constant: it used to work to do it this way

       static SEXP(*fun)(SEXP) = (SEXP(*)(SEXP)) R_GetCCallable("SomePackage", "somefunc");
      return fun(x);

but the alternate form (a litte longer) is also in use (eg in my more recent package crc32c used by digest):

      SEXP attribute_hidden c_crc32c(SEXP x) {
          static SEXP(*fun)(SEXP) = NULL;
          if (fun == NULL) fun = (SEXP(*)(SEXP)) R_GetCCallable("crc32c", "c_crc32c");
          return fun(x);
      }

You could locally edit this and I'd be happy to adjust RApiDatetime for this.

       static SEXP(*fun)(SEXP) = NULL;
       if (SEXP(*)(SEXP)) R_GetCCallable("SomePackage", "somefunc");
      return fun(x);
eddelbuettel commented 10 months ago

Ok, done in two changes.

First Change: RApiDatetime

modified   inst/include/RApiDatetime.h
@@ -2,7 +2,7 @@
 /*
  *  RApiDatetime -- Packge to provide Datetime functionality as in the R API
  *
- *  Copyright (C) 2014 - 2017  Dirk Eddelbuettel
+ *  Copyright (C) 2014 - 2023  Dirk Eddelbuettel
  *
  *  This file is part of RApiDatetime.
  *
@@ -48,34 +48,38 @@ extern "C" {
    ../src/init.c via R_RegisterCCallable()     */

 SEXP attribute_hidden asPOSIXlt(SEXP x, SEXP tz) {
-    static SEXP(*fun)(SEXP,SEXP) = (SEXP(*)(SEXP,SEXP)) R_GetCCallable("RApiDatetime", "asPOSIXlt");
+    static SEXP(*fun)(SEXP,SEXP) = NULL;
+    if (fun == NULL) fun = (SEXP(*)(SEXP,SEXP)) R_GetCCallable("RApiDatetime", "asPOSIXlt");
     return fun(x,tz);
 }

 SEXP attribute_hidden asPOSIXct(SEXP x, SEXP tz) {
-    static SEXP(*fun)(SEXP,SEXP) = (SEXP(*)(SEXP,SEXP)) R_GetCCallable("RApiDatetime", "asPOSIXct");
+    static SEXP(*fun)(SEXP,SEXP) = NULL;
+    if (fun == NULL) fun = (SEXP(*)(SEXP,SEXP)) R_GetCCallable("RApiDatetime", "asPOSIXct");
     return fun(x,tz);
 }

 SEXP attribute_hidden formatPOSIXlt(SEXP x, SEXP b, SEXP c) {
-    static SEXP(*fun)(SEXP,SEXP,SEXP) = 
-        (SEXP(*)(SEXP,SEXP,SEXP)) R_GetCCallable("RApiDatetime", "formatPOSIXlt");
+    static SEXP(*fun)(SEXP,SEXP,SEXP) = NULL;
+    if (fun == NULL) fun = (SEXP(*)(SEXP,SEXP,SEXP)) R_GetCCallable("RApiDatetime", "formatPOSIXlt");
     return fun(x,b,c);
 }

 SEXP attribute_hidden Rstrptime(SEXP x, SEXP fmt, SEXP tz) {
-    static SEXP(*fun)(SEXP,SEXP,SEXP) =
-        (SEXP(*)(SEXP,SEXP,SEXP)) R_GetCCallable("RApiDatetime", "Rstrptime");
+    static SEXP(*fun)(SEXP,SEXP,SEXP) = NULL;
+    if (fun == NULL) fun = (SEXP(*)(SEXP,SEXP,SEXP)) R_GetCCallable("RApiDatetime", "Rstrptime");
     return fun(x,fmt,tz);
 }

 SEXP attribute_hidden POSIXlt2D(SEXP x) {
-    static SEXP(*fun)(SEXP) = (SEXP(*)(SEXP)) R_GetCCallable("RApiDatetime", "POSIXlt2D");
+    static SEXP(*fun)(SEXP) = NULL;
+    if (fun == NULL) fun = (SEXP(*)(SEXP)) R_GetCCallable("RApiDatetime", "POSIXlt2D");
     return fun(x);
 }

 SEXP attribute_hidden D2POSIXlt(SEXP x) {
-    static SEXP(*fun)(SEXP) = (SEXP(*)(SEXP)) R_GetCCallable("RApiDatetime", "D2POSIXlt");
+    static SEXP(*fun)(SEXP) = NULL;
+    if (fun == NULL) fun = (SEXP(*)(SEXP)) R_GetCCallable("RApiDatetime", "D2POSIXlt");
     return fun(x);
 }

Second Change: rdatetime

modified   src/rdatetime.c
@@ -1,8 +1,7 @@

 #include "rdatetime.h"
+#include "RApiDatetime.h"

 void _rdatetime() {

-
-
 }
modified   src/rdatetime.h
@@ -1,8 +1,6 @@
 #ifndef R_RDATETIME_H
 #define R_RDATETIME_H

-#include "RApiDatetime.h"
-
 void _rdatetime();

Result

edd@rob:~/git/rdatetime(main)$ install.r
* installing *source* package found in current working directory ...
* installing *source* package ‘rdatetime’ ...
** using staged installation
** libs
using C compiler: ‘gcc (Ubuntu 13.2.0-4ubuntu3) 13.2.0’
ccache gcc -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/RApiDatetime/include'     -fpic  -g -O3 -Wall -pipe            -fdiagnostics-color=always -c init.c -o init.o
ccache gcc -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/RApiDatetime/include'     -fpic  -g -O3 -Wall -pipe            -fdiagnostics-color=always -c rdatetime.c -o rdatetime.o
ccache gcc -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -Wl,-z,relro -o rdatetime.so init.o rdatetime.o -L/usr/lib/R/lib -lR
installing to /usr/local/lib/R/site-library/00LOCK-rdatetime/00new/rdatetime/libs
** R
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (rdatetime)
edd@rob:~/git/rdatetime(main)$ 

This is slightly different from what I said above and more refined: the header remains in src/init.c as it needs to declare the function reference too in the header you include. But that header no longer includes the exporting RApiDatetime.h, only the source file does and with that you have one compilation unit with the symbol. Easy peasy.

(One can use inline to localize the included function but .... is it really needed?)

Hope this helps. I can make a maintenance release of RApiDateime if you want to use it.

eddelbuettel commented 10 months ago

PS One thing you could check is whether your client package will stop complaining with error: initializer element is not constant if the calling code is C++ rather than C. I think I only ever exported to C++ which may be less picky here.

dcooley commented 10 months ago

Great - thanks, that's very helpful.

I've made those changes locally( RApiDatetime and rdatetime ) and it's now working / compiling without issue.

As for

I can make a maintenance release of RApiDateime if you want to use it.

I'll leave this up to you. I'm not actually going to use RApiDatetime. I was just testing my understanding of how to link C routines between R packages. And following a few of your threads on StackOverflow, other github packages, and the mailing list, led me here.

eddelbuettel commented 10 months ago

I was just testing my understanding of how to link C routines between R packages.

My more recent packages on this are

It's a good trick and good to know. As you seem to be all set, I will now close. If there is something that needs change feel free to re-open.