Skip to content
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

Fix non-API calls for closures #145

Merged
merged 2 commits into from
Jul 5, 2024
Merged

Conversation

shikokuchuo
Copy link
Contributor

Fixes #144.

@spgarbet
Copy link
Member

spgarbet commented Jul 3, 2024

Thank you for your contribution. Having a PR makes moving forward a lot faster.

Couple housekeeping things, do you want to add your name to the DESCRIPTION contributor? If you do that can you bump the version number for this change, it's in VERSION and DESCRIPTION.

Will a new version proposed, the package has strong dependencies and I need to reach out to a couple other groups to get them to run their tests suites and get sign off.

@spgarbet
Copy link
Member

spgarbet commented Jul 3, 2024

Tests pass. garbetsp:~/Projects/cran/yaml$ make test
R CMD INSTALL -l build/lib build/yaml

  • installing source package ‘yaml’ ...
    ** using staged installation
    ** libs
    using C compiler: ‘gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0’
    make[1]: Entering directory '/home/garbetsp/Projects/cran/yaml/build/yaml/src'
    make[1]: Nothing to be done for 'all'.
    make[1]: Leaving directory '/home/garbetsp/Projects/cran/yaml/build/yaml/src'
    installing to /home/garbetsp/Projects/cran/yaml/build/lib/00LOCK-yaml/00new/yaml/libs
    ** R
    ** inst
    ** 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
    ** checking absolute paths in shared objects and dynamic libraries
    ** testing if installed package can be loaded from final location
    ** testing if installed package keeps a record of temporary installation path
  • DONE (yaml)
    R --vanilla -e "library(RUnit); library(yaml, lib.loc = 'build/lib'); source('build/yaml/tests/RUnit.R')"

R version 4.4.0 (2024-04-24) -- "Puppy Cup"
Copyright (C) 2024 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

library(RUnit); library(yaml, lib.loc = 'build/lib'); source('build/yaml/tests/RUnit.R')
RUNIT TEST PROTOCOL -- Wed Jul 3 08:09:21 2024


Number of test functions: 171
Number of errors: 0
Number of failures: 0

1 Test Suite :
yaml tests - 171 test functions, 0 errors, 0 failures

@spgarbet
Copy link
Member

spgarbet commented Jul 3, 2024

valgrind-test

RUNIT TEST PROTOCOL -- Wed Jul  3 08:13:45 2024 
*********************************************** 
Number of test functions: 171 
Number of errors: 0 
Number of failures: 0 

 
1 Test Suite : 
yaml tests - 171 test functions, 0 errors, 0 failures
> 
> 
==1995226== 
==1995226== HEAP SUMMARY:
==1995226==     in use at exit: 57,659,298 bytes in 10,668 blocks
==1995226==   total heap usage: 35,001 allocs, 24,333 frees, 101,535,306 bytes allocated
==1995226== 
==1995226== LEAK SUMMARY:
==1995226==    definitely lost: 0 bytes in 0 blocks
==1995226==    indirectly lost: 0 bytes in 0 blocks
==1995226==      possibly lost: 0 bytes in 0 blocks
==1995226==    still reachable: 57,659,298 bytes in 10,668 blocks
==1995226==                       of which reachable via heuristic:
==1995226==                         newarray           : 4,264 bytes in 1 blocks
==1995226==         suppressed: 0 bytes in 0 blocks
==1995226== Reachable blocks (those to which a pointer was found) are not shown.
==1995226== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==1995226== 
==1995226== For lists of detected and suppressed errors, rerun with: -s
==1995226== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@shikokuchuo
Copy link
Contributor Author

Couple housekeeping things, do you want to add your name to the DESCRIPTION contributor? If you do that can you bump the version number for this change, it's in VERSION and DESCRIPTION.

Sure, I've done that. Separately, I see the GitHub release hasn't caught up to the current CRAN version - that confused me at first.

Will a new version proposed, the package has strong dependencies and I need to reach out to a couple other groups to get them to run their tests suites and get sign off.

Sure, although this fix shouldn't result in any change in behaviour. Btw. great to see you run Valgrind.

@spgarbet
Copy link
Member

spgarbet commented Jul 3, 2024

That's odd about CRAN. There were some required fixes in that version and it's been out a long while.

Edit: I remember now, the version bump mismatch was for documentation changes.

@spgarbet
Copy link
Member

spgarbet commented Jul 3, 2024

data.table test with package installed

R version 4.4.0 (2024-04-24) -- "Puppy Cup"
Copyright (C) 2024 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> source("~/Projects/cran/data.table/tests/main.R")
getDTthreads(verbose=TRUE):
  OpenMP version (_OPENMP)       201511
  omp_get_num_procs()            12
  R_DATATABLE_NUM_PROCS_PERCENT  unset (default 50)
  R_DATATABLE_NUM_THREADS        unset
  R_DATATABLE_THROTTLE           unset (default 1024)
  omp_get_thread_limit()         2147483647
  omp_get_max_threads()          12
  OMP_THREAD_LIMIT               unset
  OMP_NUM_THREADS                unset
  RestoreAfterFork               true
  data.table is using 6 threads with throttle==1024. See ?setDTthreads.
test.data.table() running: /home/garbetsp/R/x86_64-pc-linux-gnu-library/4.4/data.table/tests/tests.Rraw.bz2
Running test id 2243.56            
Wed Jul  3 09:36:58 2024  endian==little, sizeof(long double)==16, longdouble.digits==64, sizeof(pointer)==8, TZ==unset, Sys.timezone()=='America/Menominee', Sys.getlocale()=='LC_CTYPE=en_US.UTF-8;LC_NUMERIC=C;LC_TIME=en_US.UTF-8;LC_COLLATE=en_US.UTF-8;LC_MONETARY=en_US.UTF-8;LC_MESSAGES=en_US.UTF-8;LC_PAPER=en_US.UTF-8;LC_NAME=C;LC_ADDRESS=C;LC_TELEPHONE=C;LC_MEASUREMENT=en_US.UTF-8;LC_IDENTIFICATION=C', l10n_info()=='MBCS=TRUE; UTF-8=TRUE; Latin-1=FALSE; codeset=UTF-8', getDTthreads()=='OpenMP version (_OPENMP)==201511; omp_get_num_procs()==12; R_DATATABLE_NUM_PROCS_PERCENT==unset (default 50); R_DATATABLE_NUM_THREADS==unset; R_DATATABLE_THROTTLE==unset (default 1024); omp_get_thread_limit()==2147483647; omp_get_max_threads()==12; OMP_THREAD_LIMIT==unset; OMP_NUM_THREADS==unset; RestoreAfterFork==true; data.table is using 6 threads with throttle==1024. See ?setDTthreads.', zlibVersion()==1.2.11 ZLIB_VERSION==1.2.11
10 longest running tests took 13s (48% of 27s)
      ID  time nTest
 1: 2155 3.817     5
 2: 1438 1.829   738
 3: 1437 1.471    36
 4: 1652 1.068    91
 5: 1648 1.063    91
 6: 1650 1.015    91
 7: 1223 0.932   728
 8: 2233 0.890    55
 9: 1644 0.733    91
10: 1642 0.680    91
All 11070 tests (last 2243.56) in tests/tests.Rraw.bz2 completed ok in 31.3s elapsed (36.4s cpu)

@spgarbet
Copy link
Member

spgarbet commented Jul 3, 2024

@hadley @MichaelChirico Seeking your approval for release of this patch to CRAN.

@spgarbet spgarbet self-requested a review July 3, 2024 15:30
@hadley
Copy link
Contributor

hadley commented Jul 3, 2024

@spgarbet looks fine to me, but I'd feel more confident if you added some GitHub actions to check all relevant versions of R (e.g. with usethis::use_github_action("check-full"))

@spgarbet
Copy link
Member

spgarbet commented Jul 3, 2024

Our institutions account doesn't cover doing that on github. We do have full gitlab automations. I'm looking into seeing if they have something similar that can be done using gitlab. (Also "looks fine to me" have been words I've said just before serious breakage).

@hadley
Copy link
Contributor

hadley commented Jul 3, 2024

@spgarbet I don't think that should matter for publicly available repos? IIRC it's only private repos that you need to pay for.

@spgarbet
Copy link
Member

spgarbet commented Jul 5, 2024

I set up the github action on the main branch and it's passing.

Copy link
Member

@spgarbet spgarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change looks good. Hadley has approved if it passes github checks. data.table has not responded, but I ran their test suite and got no errors.

@spgarbet spgarbet merged commit afde119 into vubiostat:master Jul 5, 2024
@MichaelChirico
Copy link
Contributor

LGTM, thanks for the ping, and thanks @shikokuchuo for the PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-API Calls
4 participants