eddelbuettel / rcpptoml

Rcpp Bindings to C++ parser for TOML files
GNU General Public License v2.0
36 stars 9 forks source link

revive? #5

Closed dpastoor closed 7 years ago

dpastoor commented 7 years ago

Hi Dirk,

I was wondering how difficult it would be to revive this, along with windows support, given the updates to the window toolchain in 3.3+

Is that possible?

I attempted to muck around however C++ is completely out of my league.

eddelbuettel commented 7 years ago

It is alive!! We use it daily at work.

i) Windows support I don't need so I don't work much on it. But you may have a point that these days it may just need a rebuild given that we have g++-4.9 in Rtools. Did you try? Can you try? ii) New TOML (date + time) types just arrived, but I will wait for the (excellent) cpptoml project by @skystrife to offer them. iii) Anything else missing?

eddelbuettel commented 7 years ago

Now that you mention it ... there is more to it. I did look into it. Apparently last summer.

It turns out that we need "real" C++11 and g++-4.9 is ever so close ... but not there. Now, you may just get lucky as we needed similar features in the very recent nanotime package where @dcdillon was mostly kindly working exactly what we need here too. Else maybe just maybe we could get by with mktime00 and gmtime_ from R which Rcpp exposes. To be seen.

But he or she who really needs Windows support should really drive this. We all are mostly just happy on Linux and OS X...

dpastoor commented 7 years ago

Oh great! I'm currently on a windows machine as my mbp is on the fritz. Happy to help you debug for windows support.

I first removed the unix_only and tried to build and got the following:

==> Rcmd.exe INSTALL --no-multiarch --with-keep.source rcpptoml

* installing to library 'C:/Program Files/R/R-3.3.2/library'
* installing *source* package 'RcppTOML' ...
** libs
c:/Rtools/mingw_64/bin/g++  -std=c++0x -I"C:/PROGRA~1/R/R-33~1.2/include" -DNDEBUG -I../inst/include/ -DCPPTOML_USE_MAP   -I"C:/Program Files/R/R-3.3.2/library/Rcpp/include" -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -mtune=core2 -c RcppExports.cpp -o RcppExports.o
c:/Rtools/mingw_64/bin/g++  -std=c++0x -I"C:/PROGRA~1/R/R-33~1.2/include" -DNDEBUG -I../inst/include/ -DCPPTOML_USE_MAP   -I"C:/Program Files/R/R-3.3.2/library/Rcpp/include" -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -mtune=core2 -c parse.cpp -o parse.o
parse.cpp: In function 'time_t local_timegm(tm*)':
parse.cpp:47:23: error: 'setenv' was not declared in this scope
     setenv("TZ", "", 1);
                       ^
parse.cpp:54:22: error: 'unsetenv' was not declared in this scope
         unsetenv("TZ");
                      ^
make: *** [parse.o] Error 1
Warning: running command 'make -f "Makevars" -f "C:/PROGRA~1/R/R-33~1.2/etc/x64/Makeconf" -f "C:/PROGRA~1/R/R-33~1.2/share/make/winshlib.mk" CXX='$(CXX1X) $(CXX1XSTD)' CXXFLAGS='$(CXX1XFLAGS)' CXXPICFLAGS='$(CXX1XPICFLAGS)' SHLIB_LDFLAGS='$(SHLIB_CXX1XLDFLAGS)' SHLIB_LD='$(SHLIB_CXX1XLD)' SHLIB="RcppTOML.dll" WIN=64 TCLBIN=64 OBJECTS="RcppExports.o parse.o"' had status 2
ERROR: compilation failed for package 'RcppTOML'
* removing 'C:/Program Files/R/R-3.3.2/library/RcppTOML'

This corresponds to the following function

// cf 'man timegm' for the workaround on non-Linux systems
inline time_t local_timegm(struct tm *tm) {
#if defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)
    // and there may be more OSs that have timegm() ...
    return timegm(tm);
#else
    char *tz = getenv("TZ");
    if (tz) tz = strdup(tz);
    setenv("TZ", "", 1);
    tzset();
    time_t ret = mktime(tm);
    if (tz) {
        setenv("TZ", tz, 1);
        free(tz);
    } else
        unsetenv("TZ");
    tzset();
    return ret;
#endif
}

As you mentioned, I had seen mention on SO of gmtime for a different time parser, however my very very naive attempt on how to override the inline function on windows failed. Any tips on what to try to see to see if mktime00 and gmtime are sufficient?

I would be happy to poke around, don't know if I'd qualify as a driver in Cpp but if you would be willing to bear with me I can do my best!

eddelbuettel commented 7 years ago

Good! So I have a fragment here where I seemingly tried to bring in a timegm() function from somewhere else ... only to have it fail at link time.

But in a very funny way I sort-of re-realized these days that Rcpp actually exports R's reimplementations of gmtime() and mktime() (under slightly different names). See this commit to Rcpp from two days ago (!!) which illustrates these via unit tests. We just need LinkingTo: Rcpp and nothing else.

dpastoor commented 7 years ago

great! so I just tried to install Rcpp from the tip and actuall got a failure (don't know if I should post this to Rcpp repo instead)

* installing *source* package 'Rcpp' ...
** libs

*** arch - i386
c:/Rtools/mingw_32/bin/g++  -I"C:/PROGRA~1/R/R-33~1.2/include" -DNDEBUG -I../inst/include/    -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -mtune=core2 -c Date.cpp -o Date.o
Date.cpp: In function 'tm* Rcpp::timesub(const time_t*, int_fast32_t, const Rcpp::state*, tm*)':
Date.cpp:1351:14: error: 'struct tm' has no member named 'tm_gmtoff'
         tmp->tm_gmtoff = offset;
              ^
make: *** [Date.o] Error 1
Warning: running command 'make -f "Makevars.win" -f "C:/PROGRA~1/R/R-33~1.2/etc/i386/Makeconf" -f "C:/PROGRA~1/R/R-33~1.2/share/make/winshlib.mk" SHLIB_LDFLAGS='$(SHLIB_CXXLDFLAGS)' SHLIB_LD='$(SHLIB_CXXLD)' SHLIB="Rcpp.dll" OBJECTS="Date.o Module.o Rcpp_init.o api.o attributes.o barrier.o"' had status 2
ERROR: compilation failed for package 'Rcpp'
...

I thought it could be my comp, so I grabbed another windows computer and also tried to install

devtools::install_github("RcppCore/Rcpp")

and got the exact same error.

Given this gets fixed, when you say just need the linkingTo, you mean that shouldn't need to change the inline function at all, just make sure to have a (working) development version of Rcpp should suffice?

eddelbuettel commented 7 years ago

Weird. I just ported that / updated that. I need to look into that.

But the good news for you is that the underlying functions have been there "forever". Just use the CRAN version. Just be careful: it is gmtime_() with an extra underscore, and mktime00() with two extra zeros.

eddelbuettel commented 7 years ago

Ok, that needs a fix and will get it. Lines 1350-ish, that end of the file, needs to be

        //#ifdef HAVE_TM_GMTOFF
#if ! (defined(__MINGW32__) || defined(__MINGW64__))
        tmp->tm_gmtoff = offset;
#endif
        //#endif
        return tmp;

Rest of the tests running now.

dpastoor commented 7 years ago

attempted fix in PR #6

eddelbuettel commented 7 years ago

And the test fails. Fiddle sticks. So Rcpp is not quite there yet with the change from last week.

dpastoor commented 7 years ago

well, if you want to ping me on changes, I'm happy to compile and give it a shot any time.

eddelbuettel commented 7 years ago

I poked around some more. Looks like Rcpp's master branch was simply a little behind testing standards on Windows; that is the price we pay for not bothering with appveyor on each commit... I should have a handle on this tomorrow with access to my windows vm.

As for RcppTOML: Let's do this. Worst case we can build something which may miss one or two features related to dates. But we should have the components. So let's bring this baby to windows.

dpastoor commented 7 years ago

awesome!! My cursory check looked like the dates parsed ok :-) If it comes to it, I'd also be happy to take the 'burden' of writing up any regression tests if we do run into bug(s)