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

Export renv (#55) #104

Merged
merged 6 commits into from
Mar 2, 2023
Merged

Export renv (#55) #104

merged 6 commits into from
Mar 2, 2023

Conversation

schochastics
Copy link
Member

@chainsawriot not entirely sure yet if this is robust but it seems to be a minimal working version. I keep digging to make this more stable. Something I am wondering about is the git stuff for bioconductor. If you see other vulnerabilities, do let me know

@chainsawriot
Copy link
Collaborator

This is the generated json for an package. I think the brackets affect renv.

    "Packages": {
        "slam": {
            "Package": [
                "slam"
            ],
            "Version": [
                "0.1-50"
            ],
            "Source": [
                "Repository"
            ],
            "Repository": [
                "CRAN"
            ]
        }
> restore()

This project has not yet been activated.
Activating this project will ensure the project library is used during restore.
Please see `?renv::activate` for more details.

Would you like to activate this project before restore? [Y/n]: y
* Project '~/dev/misc/renvtest2' loaded. [renv 0.16.0]
* The project library is out of sync with the lockfile.
* Use `renv::restore()` to install packages recorded in the lockfile.
Error in vapply(x, f, ..., FUN.VALUE = character(1)) : 
  values must be type 'character',
 but FUN(X[[1]]) result is type 'list'
Traceback (most recent calls last):
6: restore()
5: renv_restore_report_actions(diff, current, lockfile)
4: renv_pretty_print_records_pair(lhs[names(lhs) %in% names(actions)], 
       rhs[names(rhs) %in% names(actions)], "The following package(s) will be updated:")
3: renv_pretty_print_records_pair_impl(old, new)
2: map_chr(all, function(package) {
       lhs <- old[[package]]
       rhs <- new[[package]]
       case(is.null(lhs$Source) ~ rhs$Repository %||% rhs$Source, 
           is.null(rhs$Source) ~ lhs$Repository %||% lhs$Source, 
           !is.null(rhs$Repository) ~ rhs$Repository, !is.null(rhs$Source) ~ 
               rhs$Source)
   })
1: vapply(x, f, ..., FUN.VALUE = character(1))

But I was wondering what the resolve("renv") does in the function?

@chainsawriot
Copy link
Collaborator

pkg_list <- vector(mode = "list", length = 1)
pkg_list[[1]][["Package"]] <- "rtoot"
pkg_list[[1]][["Version"]] <- "0.1.0"
pkg_list[[1]][["Source"]] <- "Repository"
pkg_list[[1]][["Repository"]] <- "CRAN"
jsonlite::toJSON(pkg_list)
## [{"Package":["rtoot"],"Version":["0.1.0"],"Source":["Repository"],"Repository":["CRAN"]}] 
jsonlite::toJSON(pkg_list, auto_unbox = TRUE)
## [{"Package":"rtoot","Version":"0.1.0","Source":"Repository","Repository":"CRAN"}] 

@schochastics
Copy link
Member Author

ah I forgot to include auto_unbox this should be fixed now.

The resolve("renv") part I am actually not sure if this is needed like this. renv is also part of the renv lockfile so we could also kind of hard code it in?

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Merging #104 (a47fef5) into v0.2 (34b2f96) will decrease coverage by 1.78%.
The diff coverage is 48.38%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             v0.2     #104      +/-   ##
==========================================
- Coverage   97.06%   95.29%   -1.78%     
==========================================
  Files           6        6              
  Lines         819      850      +31     
==========================================
+ Hits          795      810      +15     
- Misses         24       40      +16     
Impacted Files Coverage Δ
R/installation.R 93.11% <48.38%> (-4.58%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@chainsawriot
Copy link
Collaborator

chainsawriot commented Mar 2, 2023

@schochastics Great! The smoke test is now working. (But it proves our point in the documentation, a lock file is not as great as a Dockerfile and rang.R. The restoration of the lock file generated by the example in the documentation doesn't work due to misconfigured rJava. And rJava is not working well with the approach renv uses. It's not an issue we can fix though.)

About the resolve("renv"), actually, it does nothing now and the smoke test still runs. And the issue is: if someone uses renv::restore() to rehydrate a lock file from someone else, renv should have been installed anyway.

@schochastics
Copy link
Member Author

sounds awesome. Yeah having renv installed to rehydrate obviously makes sense. i will remove it

@schochastics schochastics marked this pull request as ready for review March 2, 2023 17:13
@schochastics
Copy link
Member Author

@chainsawriot should be done now

@chainsawriot chainsawriot merged commit c901db5 into gesistsa:v0.2 Mar 2, 2023
@chainsawriot
Copy link
Collaborator

@schochastics Thanks a lot!

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.

3 participants