-
-
Notifications
You must be signed in to change notification settings - Fork 212
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
New module installation mechanism #5269
Conversation
I will test it on Mac! |
883b250
to
f0e1060
Compare
95bd7e7
to
4648e5b
Compare
Ah, so this does not work on macos. JASPengine depends on R-Interface which depends on RInside/Rcpp which depends on JASPEngine... |
Well, turns out that is not necesarily the problem, as the Patch.cmake would still be run.
Which then messes up this code:
So I will rewrite the calls to RInside back to R |
Im getting closer but running into trouble like:
Ive rebase the code on upgradeConan2 branch from #5245 and pushed it to: https://github.com/JorisGoosen/jasp-desktop/tree/newModuleInstall |
Well, I found some more problems, Rinside now actually gets patched and linked into jaspEngine. |
Hmm, Ive taken your branch as it is in 1d52f92a5d5ff362cb0bc5f5fb589cfa5cdd0f1d , squashed it and added some mac fixes on top to make it compile and pushed that here: https://github.com/JorisGoosen/jasp-desktop/tree/newModuleInstall however it does not run, maybe this is where I need to make some proceshelper changes?
I guess it isnt finding Rcpp?
|
I guess we should move the stuff under |
@vandenman want to integrate my changes? Or I can push to your branch perhaps |
@JorisGoosen sorry for the delay, feel free to push directly to this branch! |
1d52f92
to
2cc5d34
Compare
Ok, so the problem is that Rcpp is now not in the main R library of the framework. So in jaspRcpp:
Fails because it really depends on Rcpp (and maybe RInside haha) But that might make @vandenman sad... So if he has a better solution? |
Doesn't RInside look at an environment variable that we can set? I'll take a look tomorrow. |
8b1cc32
to
d371dca
Compare
d45fe13
to
3a5d2b8
Compare
51a94ae
to
b41342d
Compare
60469d5
to
22341ae
Compare
very basic functionality works start on making hashes work seems more or less functional now started rewriting cmake, stuff compiles but crashes at runtime everything seems functional on Linux windows fixes, stuff compiles but weird runtime error building on windows works from a clean build, but more testing is needed more tweaks, update to renv 1.0.2 updates for fallback without lockfile start unifying stuff across OSes autogenerated lockfiles seem to be work okay but need more testing reorganize file + pkgdepends fallback bump jaspModuleInstaller spaces -> tabs bump jaspModuleInstaller spaces -> tabs moved getModuleDependencies to jaspModuleInstaller call setupRenv in setup_renv_rcpp_rinside_jaspModuleInstaller.R.in remove custom Matrix installation make configuration work on macos compilation still fails though... do follow symlinks otherwise it patches nothing move searching for fortran outside of the "does framework not exist if()f" jaspModuleInstaller and fixing of r pkgs needs jaspEngine so change the order in which things are installed add R_LIBS for windows and macos make windows start
22341ae
to
7345d32
Compare
Ok so the code at https://github.com/jasp-stats/jasp-desktop/tree/lockfiletesting does compile for macos and locally it works. |
superseded by #5629 |
@RensDofferhoff I have only tested on Windows and Linux, and Windows was a few weeks ago. Could you give this a try on macOS?
I would recommend testing this PR in a new build folder because this will mess with the module library and renv-cache.
Also, in
processhelper.cpp
it's possible that macOS needs some additional changes. I just haven't made them.I still need to write some dev-docs and add more comments, so just see this as a first MWE.
All feedback is welcome!