-
Notifications
You must be signed in to change notification settings - Fork 8
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
move R code into hera
#130
base: main
Are you sure you want to change the base?
Conversation
… and calling the configure function. The package will deal with loading the R code
CMakeLists.txt
Outdated
if(EMSCRIPTEN) | ||
# | ||
else() | ||
execute_process(COMMAND ${R_COMMAND} CMD INSTALL ../hera) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be the final solution, but that gets us the current version of hera installed for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps it belongs more in the make
then the cmake
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does R CMD INSTALL do a lot more than installing files in directories? We could potentially do it through the CMAKE "install" function.
What doe you think? This would make it possible to make it work without invoking R.
CMakeLists.txt
Outdated
# Install R package hera | ||
# ====================== | ||
if(EMSCRIPTEN) | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what is involved in "installing" an R package if(EMSCRIPTEN)
I think this is in a good initial place for non EMSCRIPTEN. It might be easier if |
Thanks @SylvainCorlay for the ☎️ hints, I'll resume later, but one thing we can do perhaps is to make the files with
|
That's great. I think we should be able to get away with that without publishing hera until it fuly stabilises. |
message(STATUS "Installing R 📦 hera to temporary library") | ||
execute_process(COMMAND ${R_COMMAND} CMD INSTALL --preclean --no-staged-install --library=temp_r_lib --build ../hera) | ||
install(DIRECTORY temp_r_lib/ DESTINATION ${R_HOME}/library) | ||
execute_process(COMMAND ${R_SCRIPT_COMMAND} -e "writeLines(paste('Installed 📦 hera version', as.character(packageVersion('hera'))))") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After these, we get 3 things:
- the (binary) 📦 is installed where it needs to for local use, e.g.
${R_HOME}/library
- the (binary) 📦 is also in the
temp_r_lib/hera
directory:
(xeusr2025) romainfrancois@tada build % tree temp_r_lib/hera
temp_r_lib/hera
├── DESCRIPTION
├── INDEX
├── LICENSE
├── Meta
│ ├── Rd.rds
│ ├── features.rds
│ ├── hsearch.rds
│ ├── links.rds
│ ├── nsInfo.rds
│ └── package.rds
├── NAMESPACE
├── R
│ ├── hera
│ ├── hera.rdb
│ └── hera.rdx
├── help
│ ├── AnIndex
│ ├── aliases.rds
│ ├── hera.rdb
│ ├── hera.rdx
│ └── paths.rds
└── html
├── 00Index.html
└── R.css
5 directories, 20 files
- because of the
--build
option, we get thehera_****.tgz
file as well, if that makes it easier for EMSCRIPTING it, whatever that means :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For EMSCRIPTING, having hera
installed in ${R_HOME}/library
should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @IsabelParedes I'll simplify then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now we have two things:
- the
.tgz
but if we don't need it, we can remove the--build
option if it's not needed. - the binary 📦 installed in
{R_HOME}/library
, i.e.
(xeusr2025) romainfrancois@tada build % tree /Users/romainfrancois/miniforge3/envs/xeusr2025/lib/R/library/hera
/Users/romainfrancois/miniforge3/envs/xeusr2025/lib/R/library/hera
├── DESCRIPTION
├── INDEX
├── LICENSE
├── Meta
│ ├── Rd.rds
│ ├── features.rds
│ ├── hsearch.rds
│ ├── links.rds
│ ├── nsInfo.rds
│ └── package.rds
├── NAMESPACE
├── R
│ ├── hera
│ ├── hera.rdb
│ └── hera.rdx
├── help
│ ├── AnIndex
│ ├── aliases.rds
│ ├── hera.rdb
│ ├── hera.rdx
│ └── paths.rds
└── html
├── 00Index.html
└── R.css
5 directories, 20 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look at the failures in the Build and Deploy
workflow, but I think I'm now stuck on how to move this forward. cc @SylvainCorlay @IsabelParedes happy to jump on a call.
This is early stage. Probably does not work yet. I'll keep going tomorrow.
This will be much cleaner because the way we structured the code previously in xeus-r was essentially mimicing what an R package does, but not as good.