New method for module installation #88
Conversation
start on new module install now also uses local versions of indirect dependencies expose argument option to record packages into lockfiles also expose argument to update R package dependencies tweak file listing to not be recurse if not needed, use cat instead of print hash used in cache now also matches that in the lockfile return 'success' and some tweaks to printing fix missing file.path set libPaths inside jaspBase nvm that made no sense revamp and refactor module installation some progress use explicit snapshot type add clean=TRUE for restore fix jasp-stats/INTERNAL-jasp#1891 start on unit tests for module installation some work on the tests tests now pass locally remove unnecessary autogenerated comment patch in symlink check for R <4.2.1 patch symlink check again set copy.mode to FALSE remove and ignore renv stuff remove rlang as dependency and try to fix tests on windows paths -> x, drop R 4.1.3 just use rlang again use rlang::hash instead of tools::md5sum just debug on gh actions forgot to update one hash Update rcmdcheck.yml tempdir() -> withr::local_dir(), so separate tests use different folders remove logging from hashes, add on test-failure info for windows about LF <=> CRLF
| @@ -361,15 +364,648 @@ setupRenv <- function(moduleLibrary, modulePkg) { | |||
|
|
|||
| Sys.setenv("RENV_PATHS_LIBRARY" = moduleLibrary) | |||
|
|
|||
| print("Using the following paths:") | |||
| cat("Using the following paths:\n") | |||
There was a problem hiding this comment.
Now we can't see this in the log anymore when running a dynamic module install right?
Why would we want that?
There was a problem hiding this comment.
This should be visible in both? This code is, for now, intentionally not used when installing a dynamic module, so I'm not 100% sure though.
There was a problem hiding this comment.
But I assume that later we will reuse this code also for dynamic modules right?
Those are going to be exactly the same after all, as the ones in jasp-desktop.
But im not sure what the gain is from changing them from print anyway?
There was a problem hiding this comment.
cat sometimes results in nicer output than print. For example:
> str <- "This is bad:\nbecause this\n"
> cat(str)
This is bad:
because this
> print(str)
[1] "This is bad:\nbecause this\n"So with cat you can print with formatting like \n, \t, etc. With print it just prints \n and \t literally. Also, I checked and cat prints to stdout (or any other connection we provide).
| #' | ||
| #' @return returns \code{NULL}. | ||
| #' | ||
| installModuleNew <- function( |
There was a problem hiding this comment.
This function is so large I think it would be good to have some comments in between breaking up the flow describing what each section does. Like "Here we are going to scan the lockfile" or something.
It would help me, and probably future you, out a lot.
Splitting it up in some descriptively named functions would also be a good way to solve this but I can imagine that would be a bit painful so I wont ask for that.
|
well this is one massive set of changes :o Ive skimmed through it now only. Ill also try to build this together with jasp-desktop and see whether it works. |
This is why I didn't delete any of the old code (yet). Dynamic modules still uses that. This only concerns the build process. I can also test this with dynamic modules immediately if you prefer, but I figured the current changes have gotten large enough 😅 |
Yes which is good, the keeping of the old code that is, but there could still be problems with the links in the libraries in the build/install-folder. What could happen is that jaspBase or jaspModules library links to |
JorisGoosen
force-pushed
the
master
branch
2 times, most recently
from
8b639a0
to
7bd7444
Compare
Fixes https://github.com/jasp-stats/INTERNAL-jasp/issues/1887
part of https://github.com/jasp-stats/INTERNAL-jasp/issues/1782
See jasp-stats/jasp-desktop#4884 for an explanation.
This needs to be tested extensively.