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

Underlying TRNG C++ library updated to v4.23(.1) #20

Merged
merged 7 commits into from
Jan 29, 2021

Conversation

riccardoporreca
Copy link
Member

@riccardoporreca riccardoporreca commented Jan 26, 2021

See #18 for the intended upgrade to v4.23, and #21 for the the inclusion of the patch release v4.23.1, which fixes #16

In particular, the efforts described in #16 around reproducing and assessing valgrind issues based on the Docker image wch1/r-debug have been consolidated into a new GH actions workflow to automate such checks.

* Via inst/tools/upgradeTRNG.R, extended to handle fetching TRNG from a GitHub release tag.
* Align to the outcome of the current version of `usethis::use_coverage()`.
* This makes the status checks informational, see https://docs.codecov.io/docs/commit-status for reference.
@riccardoporreca
Copy link
Member Author

This also includes a small maintenance on the codecov side to align to the standard settings one would obtain with usethis::use_coverage(), which also make the coverage diff only informative (i.e. not shown as failures)

Copy link
Member

@RolandASc RolandASc left a comment

Choose a reason for hiding this comment

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

looks good to me

@riccardoporreca riccardoporreca changed the title Underlying TRNG C++ library updated to v4.23 Underlying TRNG C++ library updated to v4.23(.1) Jan 28, 2021
@riccardoporreca
Copy link
Member Author

@RolandASc, I am going to integrate the changes done in a patch of TRNG 4.23 upstream (see #16 (comment)) in this branch, so we include the patched 4.23.1 in this upgrade

* Based on the docker image `wch1/r-debug` and inspired by the CRAN checks logs for valgrind.
* An artifact is produced containing the R CMD check output directory + a summary of the valgrind reported errors by file.
* A failure is detected whenever a non-0 errors are found in any file.
* The workflow is triggered on push and PRs on master, but can be triggered on any commit using [valgrind-check] in the comment.
* The core bash script can be also executed locally, producing check results under `valgrind-check` (`.git`- and `.Rbuildignore`d).;
* Used to test backporting as a local patch of v4.23 the upstream TRNG fix rabauke/trng4@22cc3b6 addressing uninitialized memory issues reported by `valgrind` (#16), ahead of a proper patch release being available upstream.
* The process is made reproducible by extending inst/tools/upgradeTRNG.R to consume and apply a patch file, adding a patch component to the version (e.g. 4.23.1), with an explicit mention in `TRNG.Version()` about this being a version patched inside rTRNG.

[valgrind-check]
* Via inst/tools/upgradeTRNG.R
* The patch release fixes the uninitialized-memory problems reported as `valgrind` issues in the CRAN package checks for rTRNG 4.20-1 (closes #16).

[valgrind-check]
* To ensure the workflow is executed on PRs to master, w/o requiring a [valgrind-check] commit message, as originally intended.
* Via inst/tools/upgradeTRNG.R, extended to rely on the GitHub release tag for newer versions, and exposing a `year` argument.
@riccardoporreca
Copy link
Member Author

@RolandASc, I am going to merge this after including #21, I don't expect many functional aspects of the additional changed to be reviewed at this stage, and a final review will be done on master in preparation from the CRAN release.

@riccardoporreca riccardoporreca merged commit 62139fb into master Jan 29, 2021
@riccardoporreca riccardoporreca deleted the feature/18-trng-4.23 branch July 1, 2021 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Valgrind issues on CRAN
2 participants