update time_t to use the Rcpp mktime00 function on on non-unix system… #6

Closed
wants to merge 3 commits into
from

Projects

None yet

2 participants

@dpastoor
Contributor
dpastoor commented Jan 4, 2017

…s, uncure if removing the timezone setting is incorrect, however get/setenv were giving errors.

@dpastoor dpastoor update time_t to use the Rcpp mktime00 function on on non-unix system…
…s, uncure if removing the timezone setting is incorrect, however get/setenv were giving errors.
994bcaf
@dpastoor
Contributor
dpastoor commented Jan 4, 2017

Result of running R CMD CHECK passed

Setting env vars ---------------------------------------------------------------
_R_CHECK_CRAN_INCOMING_ : FALSE
_R_CHECK_FORCE_SUGGESTS_: FALSE
Checking RcppTOML --------------------------------------------------------------
"C:/PROGRA~1/R/R-33~1.2/bin/x64/R" --no-site-file --no-environ --no-save  \
  --no-restore --quiet CMD check  \
  "C:\Users\devin\AppData\Local\Temp\RtmpuwCTsp/RcppTOML_0.0.5.tar.gz"  \
  --as-cran --timings --no-manual 

* using log directory 'C:/Users/devin/Documents/Repos/RcppTOML.Rcheck'
* using R version 3.3.2 (2016-10-31)
* using platform: x86_64-w64-mingw32 (64-bit)
* using session charset: ISO8859-1
* using options '--no-manual --as-cran'
* checking for file 'RcppTOML/DESCRIPTION' ... OK
* checking extension type ... Package
* this is package 'RcppTOML' version '0.0.5'
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking whether package 'RcppTOML' can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* loading checks for arch 'i386'
** checking whether the package can be loaded ... OK
** checking whether the package can be loaded with stated dependencies ... OK
** checking whether the package can be unloaded cleanly ... OK
** checking whether the namespace can be loaded with stated dependencies ... OK
** checking whether the namespace can be unloaded cleanly ... OK
* loading checks for arch 'x64'
** checking whether the package can be loaded ... OK
** checking whether the package can be loaded with stated dependencies ... OK
** checking whether the package can be unloaded cleanly ... OK
** checking whether the namespace can be loaded with stated dependencies ... OK
** checking whether the namespace can be unloaded cleanly ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd line widths ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking line endings in C/C++/Fortran sources/headers ... OK
* checking line endings in Makefiles ... OK
* checking compilation flags in Makevars ... OK
* checking for GNU extensions in Makefiles ... OK
* checking for portable use of $(BLAS_LIBS) and $(LAPACK_LIBS) ... OK
* checking compiled code ... OK
* checking examples ...
** running examples for arch 'i386' ... OK
** running examples for arch 'x64' ... OK
* checking for unstated dependencies in 'tests' ... OK
* checking tests ...
** running tests for arch 'i386' ...
  Running 'arrays.R'
  Comparing 'arrays.Rout' to 'arrays.Rout.save' ... OK
  Running 'bool_datetime.R'
  Comparing 'bool_datetime.Rout' to 'bool_datetime.Rout.save' ... OK
  Running 'float.R'
  Comparing 'float.Rout' to 'float.Rout.save' ... OK
  Running 'integer.R'
  Comparing 'integer.Rout' to 'integer.Rout.save' ... OK
  Running 'strings.R'
  Comparing 'strings.Rout' to 'strings.Rout.save' ... OK
  Running 'tables.R'
  Comparing 'tables.Rout' to 'tables.Rout.save' ... OK
  Running 'tomlExamples.R'
  Comparing 'tomlExamples.Rout' to 'tomlExamples.Rout.save' ... OK
 OK
** running tests for arch 'x64' ...
  Running 'arrays.R'
  Comparing 'arrays.Rout' to 'arrays.Rout.save' ... OK
  Running 'bool_datetime.R'
  Comparing 'bool_datetime.Rout' to 'bool_datetime.Rout.save' ... OK
  Running 'float.R'
  Comparing 'float.Rout' to 'float.Rout.save' ... OK
  Running 'integer.R'
  Comparing 'integer.Rout' to 'integer.Rout.save' ... OK
  Running 'strings.R'
  Comparing 'strings.Rout' to 'strings.Rout.save' ... OK
  Running 'tables.R'
  Comparing 'tables.Rout' to 'tables.Rout.save' ... OK
  Running 'tomlExamples.R'
  Comparing 'tomlExamples.Rout' to 'tomlExamples.Rout.save' ... OK
* DONE
Status: OK
 OK



R CMD check results
0 errors | 0 warnings | 0 notes

R CMD check succeeded
src/parse.cpp
- } else
- unsetenv("TZ");
- tzset();
+ time_t ret = Rcpp::mktime00(*tm);
@dpastoor
dpastoor Jan 4, 2017 Contributor

@eddelbuettel I blew away the getenv/setenv() components as I'm getting an undeclared identifier error for both?

@eddelbuettel
eddelbuettel Jan 4, 2017 Owner

We can get setenv() / getenv() from somewhere else. That has come up before.

src/parse.cpp
- } else
- unsetenv("TZ");
- tzset();
+ time_t ret = Rcpp::mktime00(*tm);
@eddelbuettel
eddelbuettel Jan 4, 2017 Owner

It's a fair idea but we may need the TZ variable... Let me give this some more thought.

But a very good first step.

@eddelbuettel
eddelbuettel Jan 4, 2017 Owner

Different route: I just defined another elif defined(__MINGW32__) || defined(__MINGW32__) and in that just call mktime00() as you do here. Seems to work.

@dpastoor dpastoor referenced this pull request Jan 4, 2017
Closed

revive? #5

@dpastoor

@eddelbuettel like this? Wouldn't we still want a trailing else to fall back on something?

Also, will mktime00 handle tz() properly (it looks like it does in the cursory test checks you have)

@eddelbuettel
Owner

I think localtime seeps through, as we are coming from R, we are generally set.

@eddelbuettel
Owner

Can you make your change to parse.cpp be this:

// 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);
#elif defined(__MINGW32__) || defined(__MINGW64__)
    return Rcpp::mktime00(*tm);  // Rcpp exports a copy of the R-internal function
#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
}
@eddelbuettel
Owner

I merged by hand. Your two commits are in.

@dpastoor
Contributor
dpastoor commented Jan 4, 2017

great! I also just built and ran the tomlExamples related to time and worked great!

@eddelbuettel
Owner

Yep, I ran those too just like you and I think we're good. Will commit in a moment into a new branch (or, rather, the stale branch I had here for six months), then try to catch up to cpptoml upstream ... and with that head towards a 0.1.0 releases.

This has been a really useful chat!

@dpastoor
Contributor
dpastoor commented Jan 4, 2017

Thank you so much for your work - really looking forward to being able to use this instead of yaml!

@eddelbuettel eddelbuettel added a commit that referenced this pull request Jan 4, 2017
@eddelbuettel refinement to the previous change
stems from parallel discussions and tests with @dpastoor in #5 and #6
06b075e
@eddelbuettel
Owner

This PR is now indirectly merge (crediting two of your commits) but I wanted in a branch first for more testing and changes...

@dpastoor dpastoor deleted the dpastoor:windows branch Jan 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment