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

New module installation mechanism #5269

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

vandenman
Copy link
Contributor

@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!

@RensDofferhoff
Copy link
Contributor

I will test it on Mac!

@JorisGoosen
Copy link
Contributor

Ah, so this does not work on macos.

JASPengine depends on R-Interface which depends on RInside/Rcpp which depends on JASPEngine...
I think the only solution is to split off the otoolstuff into a separate application.
And outside of JASP in pure R land it isnt necessary either.

@JorisGoosen
Copy link
Contributor

Well, turns out that is not necesarily the problem, as the Patch.cmake would still be run.
But another problem is that RScript is used, which I think adds back this warning:

WARNING: ignoring environment value of
  R_HOME

Which then messes up this code:

if(NOT EXISTS ${RCPP_PATH})
	message(FATAL_ERROR "'Rcpp' installation has failed! ${RCPP_PATH}")
endif()
if(NOT EXISTS ${RINSIDE_PATH})
    message(FATAL_ERROR "'RInside' installation has failed!")
endif()

So I will rewrite the calls to RInside back to R

@JorisGoosen
Copy link
Contributor

Im getting closer but running into trouble like:

[1] "Installed jaspGraphs to '/Users/joris/Broncode/JASP/build-jasp-desktop-Qt_6_5_3_for_macOS-Debug/Modules/jaspTTests/.renv/1/jaspGraphs', now running post install fixes."
dyld[54036]: Library not loaded: libRInside.dylib
  Referenced from: <9C088926-30D4-3379-9AE5-3DB8D02E9EB5> /Users/joris/Broncode/JASP/build-jasp-desktop-Qt_6_5_3_for_macOS-Debug/Desktop/JASPEngine
  Reason: tried: 'libRInside.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OSlibRInside.dylib' (no such file), 'libRInside.dylib' (no such file), '/Users/joris/Broncode/JASP/build-jasp-desktop-Qt_6_5_3_for_macOS-Debug/Frameworks/R.framework/Versions/4.3-arm64/Resources/libRInside.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/Users/joris/Broncode/JASP/build-jasp-desktop-Qt_6_5_3_for_macOS-Debug/Frameworks/R.framework/Versions/4.3-arm64/Resources/libRInside.dylib' (no such file), '/Users/joris/Broncode/JASP/build-jasp-desktop-Qt_6_5_3_for_macOS-Debug/Frameworks/R.framework/Versions/4.3-arm64/Resources/libRInside.dylib' (no such file)
Error: install of package 'jaspGraphs' failed
In addition: Warning messages:
1: In generateBasicLockfile(lockfilePath, modulePkg, moduleLibrary) :
  Creating a basic lockfile by installing the module and unnecessarily installing GitHub dependencies. Please manage this lockfile yourself!
2: In getRecordsFromPkgdepends(modulePaths) :
  getRecordsFromPkgdepends failed!
3: In stop(e, domain = NA) : additional arguments ignored in stop()

Ive rebase the code on upgradeConan2 branch from #5245 and pushed it to: https://github.com/JorisGoosen/jasp-desktop/tree/newModuleInstall

@JorisGoosen
Copy link
Contributor

Well, I found some more problems, Rinside now actually gets patched and linked into jaspEngine.
However, the module folders are still empty for some reason and also Rcpp cant be found during runtime.
But that is something for next week

@JorisGoosen
Copy link
Contributor

JorisGoosen commented Oct 25, 2023

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?
But I have no good oversight on what all exactly changed :p

Engine#0:	Entering jaspRCPP_init.
Creating RInside.
Error: function 'Rcpp_precious_remove' not provided by package 'Rcpp'
In addition: Warning message:
In library(package, lib.loc = lib.loc, character.only = TRUE, logical.return = TRUE,  :
  there is no package called ‘Rcpp’
Execution halted

I guess it isnt finding Rcpp?
But im passing the main library and all, I guess there are libraries hidden somewhere else?

R_CPP_INCLUDES_LIBRARY = /Users/joris/Broncode/JASP/build-jasp-desktop-Qt_6_5_3_for_macOS-Debug/R/R_cpp_includes_library is probably it yeah
alright, Ill see how to get it in the processhelper

@JorisGoosen
Copy link
Contributor

I guess we should move the stuff under build/R to build/Modules at least on macOs
because otherwise we have to figure out exactly how to fit this into the bundle/framework structure all over again

@JorisGoosen
Copy link
Contributor

@vandenman want to integrate my changes? Or I can push to your branch perhaps

@vandenman
Copy link
Contributor Author

@JorisGoosen sorry for the delay, feel free to push directly to this branch!

@JorisGoosen
Copy link
Contributor

Ok, so the problem is that Rcpp is now not in the main R library of the framework.

So in jaspRcpp:

	rinside = new RInside();

Fails because it really depends on Rcpp (and maybe RInside haha)
In any case, the easiest solution would be to install Rcpp to the main library anyway.

But that might make @vandenman sad... So if he has a better solution?
.libPaths cant work cause R isnt loaded yet...

@vandenman
Copy link
Contributor Author

Doesn't RInside look at an environment variable that we can set? I'll take a look tomorrow.

@vandenman
Copy link
Contributor Author

Ok, so for me the current commit just works... I have an error with installing xml2 but that hopefully resolves itself. The main difference I think is that I have this when I print the libPaths:

image

we probably just need to set an env variable somewhere.

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
@JorisGoosen
Copy link
Contributor

Ok so the code at https://github.com/jasp-stats/jasp-desktop/tree/lockfiletesting does compile for macos and locally it works.
But the x86 dmg nightly gives this on startup:
afbeelding

@vandenman vandenman requested review from RensDofferhoff and removed request for RensDofferhoff July 16, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants