rix: Reproducible Environments with Nix #625
Comments
|
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type |
|
🚀 Editor check started 👋 |
Checks for rix (v0.6.0)git hash: e7b673cb
Important: All failing checks above must be addressed prior to proceeding Package License: GPL (>= 3) 1. Package DependenciesDetails of Package Dependency Usage (click to open)
The table below tallies all function calls to all packages ('ncalls'), both internal (r-base + recommended, along with the package itself), and external (imported and suggested packages). 'NA' values indicate packages to which no identified calls to R functions could be found. Note that these results are generated by an automated code-tagging system which may not be entirely accurate.
Click below for tallies of functions used in each package. Locations of each call within this package may be generated locally by running 's <- pkgstats::pkgstats(<path/to/repo>)', and examining the 'external_calls' table. baselapply (15), Sys.getenv (14), is.null (11), paste0 (9), Filter (8), file.path (7), list (7), names (7), for (6), get (6), grep (6), paste (6), seq_along (6), unlist (6), vapply (6), all (4), c (4), grepl (4), if (4), nzchar (4), args (3), character (3), emptyenv (3), switch (3), any (2), as.list (2), call (2), environmentName (2), getOption (2), gsub (2), is.function (2), new.env (2), setdiff (2), Sys.which (2), url (2), as.character (1), as.name (1), bquote (1), deparse (1), do.call (1), file (1), file.exists (1), formals (1), format (1), identical (1), length (1), logical (1), Map (1), match (1), match.call (1), normalizePath (1), readRDS (1), source (1), split (1), strsplit (1), Sys.info (1), Sys.time (1), system.file (1), tempdir (1), unname (1) rixclassify_globals (3), deparse_chr1 (3), get_sri_hash_deps (3), detect_versions (2), find_rev (2), get_globals_exprs (2), get_rPackages (2), get_rprofile_text (2), is_nix_rsession (2), is_rstudio_session (2), nix_build_exit_msg (2), set_nix_path (2), where (2), available_r (1), check_expr (1), create_shell_nix (1), detect_os (1), fetchgit (1), fetchgits (1), fetchpkgs (1), fetchzip (1), fetchzips (1), fix_ld_library_path (1), generate_git_archived_packages (1), generate_header (1), generate_locale_archive (1), generate_locale_variables (1), generate_rix_call (1), generate_rpkgs (1), generate_rstudio_pkgs (1), generate_shell (1), generate_system_pkgs (1), generate_tex_pkgs (1), get_expr_extra_pkgs (1), get_latest (1), get_system_pkgs (1), is_empty (1), is_integerish (1), message_rprofile (1), nix_build (1), nix_build_installed (1), nix_rprofile (1), nix_shell_available (1), poll_sys_proc_nonblocking (1), quote_rnix (1), recurse_find_check_globals (1), serialize_globals (1), serialize_pkgs (1), with_assign_vec_call (1), with_assign_vecnames_call (1) codetoolsfindGlobals (3), checkUsage (2) httrGET (3), content (1) utilstimestamp (2), packageVersion (1) sysexec_status (2) jsonlitefromJSON (1) 2. Statistical PropertiesThis package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing. Details of statistical properties (click to open)
The package has:
Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages
All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by the The final measure (
2a. Network visualisationClick to see the interactive network visualisation of calls between objects in package 3.
|
| id | name | conclusion | sha | run_number | date |
|---|---|---|---|---|---|
| 7802080928 | devtools-tests-via-r-nix | success | e7b673 | 69 | 2024-02-06 |
| 7802080935 | nix-builder | success | e7b673 | 385 | 2024-02-06 |
| 7802119374 | pages build and deployment | success | ef12c6 | 276 | 2024-02-06 |
| 7802080938 | pkgdown | success | e7b673 | 532 | 2024-02-06 |
| 7802080934 | R-CMD-check | success | e7b673 | 528 | 2024-02-06 |
| 7801340546 | ropensci-pkgcheck | failure | 27a828 | 64 | 2024-02-06 |
| 7802080931 | tests-r-via-system | success | e7b673 | 66 | 2024-02-06 |
3b. goodpractice results
R CMD check with rcmdcheck
R CMD check generated the following note:
- checking package subdirectories ... NOTE
Problems with news in ‘NEWS.md’:
Cannot extract version info from the following section titles:
rix (development version)
Test coverage with covr
ERROR: Test Coverage Failed
Cyclocomplexity with cyclocomp
The following functions have cyclocomplexity >= 15:
| function | cyclocomplexity |
|---|---|
| with_nix | 27 |
| rix | 22 |
Static code analyses with lintr
lintr found the following 108 potential issues:
| message | number of times |
|---|---|
| Avoid library() and require() calls in packages | 19 |
| Lines should not be more than 80 characters. | 87 |
| unexpected '/' | 1 |
| Use <-, not =, for assignment. | 1 |
Package Versions
| package | version |
|---|---|
| pkgstats | 0.1.3.9 |
| pkgcheck | 0.1.2.14 |
Editor-in-Chief Instructions:
Processing may not proceed until the items marked with ✖️ have been resolved.
|
@ropensci-review-bot assign @ldecicco-USGS as editor |
|
I'm sorry human, I don't understand that. You can see what commands I support by typing:
|
|
@ropensci-review-bot assign @ldecicco-USGS as editor |
|
Assigned! @ldecicco-USGS is now the editor |
Editor checks:
Editor commentsLooks good! I'll begin searching for editors. |
|
@ropensci-review-bot seeking reviewers |
|
Please add this badge to the README of your package repository: [](https://github.com/ropensci/software-review/issues/625)Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news |
done with: b-rodrigues/rix@859fd18 |
|
@ropensci-review-bot assign @wdwatkins as reviewer |
|
@wdwatkins added to the reviewers list. Review due date is 2024-03-06. Thanks @wdwatkins for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
|
@wdwatkins: If you haven't done so, please fill this form for us to update our reviewers records. |
|
@ropensci-review-bot assign @assignUser as reviewer |
|
@assignUser added to the reviewers list. Review due date is 2024-03-11. Thanks @assignUser for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
|
@assignUser: If you haven't done so, please fill this form for us to update our reviewers records. |
|
📆 @wdwatkins you have 2 days left before the due date for your review (2024-03-06). |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Estimated hours spent reviewing:6
Review CommentsDue to my organization's IT policies, I can only run You may need to update the Good selection of vignettes, help files are complete. I also found your "What is nix" blog post particularly helpful for understanding what was actually happening under the hood. It seems likely to me that a good portion of your users will also be using From goodpractice resultsIs there a reason for using code commentsI see a couple 'list versions' functions rely on a stored data file. Is there a way to get this information directly from the nix archive so you don't need to keep that up to date? There are a fair number of internal functions that don't have inputs/outputs documented, I would suggest documenting those for your own/future maintainer's benefit down the road. Also, several todos still in comments. test commentsSeems like get_latest.R could be tested at least that it doesn't error and returns a hash, Re: coverage—just to make sure I understand, the 25% coverage value is ignoring the tests |
|
📆 @assignUser you have 2 days left before the due date for your review (2024-03-11). |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Estimated hours spent reviewing: 6
Review Comments{rix} is a very interesting package that will allow R developers to make use of nix powerful features without having to learn (much) of its arcane secrets 🧙 . For me personally it was a great opportunity to finally get to know 'nix' (special thanks for pointing to the DS installer as I have previously destroyed a shell env via half-removed nix 😁). While it is afaik not required by rOpenSci, I would recommend to change the default branch from 'master' to 'main'. It's a small, easy change that is well supported by GitHub but is quite meaningful. DocumentationOverall the documentation is very good and covers basic as well as advanced features of the package and points to a plethora of external resources about nix. The README especially guides both beginner and advanced users and points to the specific useful further materials. I have made a few notes while reading/following along with the vignettes that I will list as bullet points for brevity. These are all what I would consider 'nits'/nice to haves and not show stoppers.
Code
|
|
Thank you very much for your reviews! Until when do we have to address them? |
|
Thanks so much @wdwatkins and @assignUser ! There's no specific due date for a response. The sooner you respond, the fresher the comments are in the reviewers' mind. |
|
@ropensci-review-bot submit review #625 (comment) time 6 |
|
Logged review for assignUser (hours: 6) |
|
@ropensci-review-bot submit review #625 (comment) time 6 |
|
Logged review for wdwatkins (hours: 6) |
ropensci-review-bot
added
4/review(s)-in-awaiting-changes
and removed
3/reviewer(s)-assigned
labels
|
Thanks a lot @wdwatkins and @assignUser also from my side for the encouraging review feedback and concrete tips to improve {rix}, and @ldecicco-USGS for guiding the review process. |
|
@b-rodrigues, @philipp-baumann: please post your response with Here's the author guide for response. https://devguide.ropensci.org/authors-guide.html |
|
Sorry we've been slow to respond, but you'll hear from us this week! |
I’ve added a new section to the "Getting started vignette" that includes the introductory text of the blog post: b-rodrigues/rix@7542610
No, fixed with: b-rodrigues/rix@8ed3a4a
It would always be the same data, so we thought it might be better to provide that information with the package as a dataset. Also, it would work without an active internet connection.
Indeed, this will be done in the coming days.
These todos are more for future references of functionalities we want to add, but agreed, this should be moved to specific issues. Will address this in the coming days as well.
Test added with: b-rodrigues/rix@14632fe
Actually, the issue is more complex: we do have our tests running on Github Actions with nix installed (and also without, to mimick environments with and without nix), but for some complex reason, {covr} doesn’t run the tests that require Nix, these end up failing. So can’t compute the coverage. We’ve collected the commits in this pull request: b-rodrigues/rix#153 and we’ll likely add more commits to address @assignUser points as well |
|
regarding your suggestions for GHAs:
Do you happen to perhaps have a couple examples on hand I could take inspiration from? I’m struggling a bit |
Same answer :)
thanks, solved with b-rodrigues/rix@922a2c0
You may be right, but I think that the trade-off is worth it; fusen takes care of many annoying aspects of package development. But to meet devs not familiar with fusen halfway, I’ve changed
thanks, solved with b-rodrigues/rix@854a202
The idea was for users to very quickly see and understand that we expect them to read the documentation in a certain order. At first I tried using numbers, but numbers can’t be the first character of a vignette’s name. I think that if we remove them, it’ll be harder for new users to follow.
This is required because the underlying Nix function I’m not sure I understand this point:
Here is a gist with a working example: https://gist.github.com/b-rodrigues/8c4e4cc1e225c5ba7d7780066120fe0a We’ll address the other comments later this week |
{fusen} does that automatically, and after contacting the author, there is no way (for now, at least) to easily set "echo" to FALSE.
We use "unstable" throughout the package, because this is where packages are the most up-to-date. "unstable" is a bit of a misnomer though, what this means is simply that these packages aren't yet part of a point-release. But they're reviewed before being merged to unstable, so unstable provides quite stable packages (there are some exceptions of course, as nothing is perfect :) )
I'll have to think about this some more. My take is that I would like to avoid encouraging behaviours that could lead to generating development environments that can't be reproduced anymore. If the commit is set to the latest one automatically each time the function runs, then that introduces a side-effect (on top of generating a file) that I think could lead to more problems than solutions. |
What would be the best way to do so? Is there some way that we could add this without needing to touch the overall structure of the function? |
When I started |
Exactly, it's another way to get feedback on possible scoping problems of variables etc. There is minor things we can deal with and runs in code exported to the nix session: for example, we support when an |
I see there are advantages for {fusen} for smaller packages, but as things grow and we mainly want to take away complexity, I agree with @assignUser and am in favor of completely removing it in the workflow. There is |
Use a wrapper function around |
I think I disagree but am not super firm on it, if this is your choice you could output the correct shorthand with a fixed sha in an error instead of a warning. That way the user experience is still much better than manually editing generated files (which just seems so counter to the mission of {rix}) while still enforcing 'nix'ness' :D |
|
@b-rodrigues for better notification :)
Ok in that case leave branch out as it's redundant or allow adding repo + branch only and fetch the HEAD of the branch for the user. Looking at master I still see the stopifnot that you removed in the linked commit above? With regards to GHA examples you can take a look in https://github.com/apache/arrow/tree/main/.github/workflows we have lot's of workflows that do a lot of things ^^ for more piece meal examples I'd recommend the docs. You can also ping me on a PR with specific questions if you are still stuck :) |
|
@ldecicco-USGS I was asked to provide some feedback. I do have some comments (see reply in mastodon) but there might be more if I look into the code. I am not sure if it is best to post it during the review or once it ends. |
|
Hi @llrs ! Thanks for the offer. We do have 2 reviewers' already, I think creating an issue on the nix repository would probably be the best plan. @philipp-baumann and @b-rodrigues are you at a point where I should be verifying that the reviewers are satisfied with your response to the reviews? It looks like there's still some back and forth happening (?), but mostly ready? |
|
Hi we still need some time to work out some of the input from the reviewers, but I think we should be done by the end of the month at the latest. Thanks for your patience! |
So I've tried with the following commits: is that ok like this? |
Yes, that looks good 👍 |
Submitting Author Name: Bruno Rodrigues
Submitting Author Github Handle: @b-rodrigues
Other Package Authors Github handles: @philipp-baumann
Repository: https://github.com/b-rodrigues/rix
Version submitted: 0.60
Submission type: Standard
Editor: @ldecicco-USGS
Reviewers: @wdwatkins, @assignUser
Archive: TBD
Version accepted: TBD
Language: en
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
The package makes it easy to define
default.nixfiles which can then be use by the Nix package manager to build a reproducible development environment. This is especially useful to make workflows completely reproducible.This package will be useful to anyone with very strict reproducibility requirements.
Not quite: to replace what
{rix}provides one would need to combine Docker and {renv} or {groundhog}.Not applicable.
#624
@jhollist @mpadge
pkgcheckitems which your package is unable to pass.We are unable to have the coverage of 75%. This is due to several issues:
skip_on_covr()function, but on only on GA. On our computers,covr::package_coverage()runs and these tests are successfully skipped.covr.We have 26 tests, which all pass on different configurations: on Ubuntu, macOS and Windows, with or without Nix available (some tests get skipped if Nix is not available) and whether R itself is installed with Nix as well, or not (in other words, through the usual means for the operating system in use).
Because of these issues, our coverage is quite low:
but if our snapshot tests (for the functions
rix(), andtar_nix_ga()) would not be ignored, and ifcovrwould successfully run the tests forwith_nix(), our coverage would be more than 50%, we think. Some functions are very difficult to test, for exampleget_latest(), which gets the latest commit hash of thenixpkgsgithub repository.Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
Do you intend for this package to go on CRAN?
Do you intend for this package to go on Bioconductor?
Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: