-
Notifications
You must be signed in to change notification settings - Fork 17
Feature : Enable LLDB Debugging for R Dev Container #260
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
Conversation
hturner
left a comment
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 think you are still missing an update to which_r. This script should manage switching between different versions of R and update any places where the path to the R version that should be used in R terminals is hard-coded.
At the moment which_r edits both r.term.linux and r.path.linux
Lines 63 to 64 in 46b8b6c
| updated_settings_data=$(cat "$settings_file_path" | jq --arg subdir "$selected_version" '."r.rterm.linux"=$subdir | ."r.rpath.linux"=$subdir') | |
| echo "$updated_settings_data" > "$settings_file_path" |
Now we are using launch.sh, r.rterm.linux should always stay the same (with r.rpath.linux updated as before), but there are other parts of the code that which_r should change. In particular, I think it should update launch.sh.
.devcontainer/devcontainer.json
Outdated
| "r.rterm.linux": "/workspaces/r-dev-env/scripts/launch_r.sh", | ||
| "r.rpath.linux": "/workspaces/r-dev-env/build/r-devel/bin/R", |
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.
r.rpath.linux should be /usr/bin/R here as this is the only version of R that exists to begin with.
| "r.rterm.linux": "/workspaces/r-dev-env/scripts/launch_r.sh", | |
| "r.rpath.linux": "/workspaces/r-dev-env/build/r-devel/bin/R", | |
| "r.rterm.linux": "/workspaces/r-dev-env/scripts/launch_r.sh", | |
| "r.rpath.linux": "/usr/bin/R", |
.devcontainer/devcontainer.json
Outdated
| } | ||
| }, | ||
| "postCreateCommand": "find . -wholename '*.git*' -type d -prune -o -type f -exec chown vscode:vscode {} \\; && sh /workspaces/r-dev-env/scripts/localscript.sh" | ||
| "postCreateCommand": "find . -wholename '.git*' -type d -prune -o -type f -exec chown vscode:vscode {} \\; && \\\n gcc -shared -fPIC -o /workspaces/r-dev-env/scripts/allow_ptrace.so /workspaces/r-dev-env/scripts/allow_ptrace.c && \\\n chmod +x /workspaces/r-dev-env/scripts/launch_r.sh && \\\n sh /workspaces/r-dev-env/scripts/localscript.sh" |
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.
It is a good idea to compile allow_ptrace.c and change permissions on launch_r.sh as part of the setup. But we should avoid hard-coding the paths as much as possible as the root is different on GitPod and will be different if someone clones the repo to a folder named something other than r-dev-env. I would try to do these steps as part of localscript.sh and then use a relative path:
| "postCreateCommand": "find . -wholename '.git*' -type d -prune -o -type f -exec chown vscode:vscode {} \\; && \\\n gcc -shared -fPIC -o /workspaces/r-dev-env/scripts/allow_ptrace.so /workspaces/r-dev-env/scripts/allow_ptrace.c && \\\n chmod +x /workspaces/r-dev-env/scripts/launch_r.sh && \\\n sh /workspaces/r-dev-env/scripts/localscript.sh" | |
| "postCreateCommand": "find . -wholename '.git*' -type d -prune -o -type f -exec chown vscode:vscode {} \\; && \\\n sh ./scripts/localscript.sh" |
If this doesn't work, maybe we can use $PWD/scripts/localscript.sh instead.
| { | ||
| "version": "0.2.0", | ||
| "configurations": [ | ||
| { | ||
| "name": "Debug R (C debugging)", | ||
| "type": "cppdbg", | ||
| "request": "launch", | ||
| "program": "/workspaces/r-dev-env/scripts/launch_r.sh", | ||
| "args": ["/workspaces/r-dev-env/build/r-devel/bin/R"], | ||
| "stopAtEntry": false, | ||
| "cwd": "${workspaceFolder}", | ||
| "environment": [], | ||
| "externalConsole": false, | ||
| "MIMode": "lldb" | ||
| } | ||
| ] | ||
| } |
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 am not sure why you are using a different launch process from the one proposed. Maybe we can offer alternative debugging configurations if they offer a different debugging experience, or provide some robustness (maybe one works if the other doesn't).
However, you should avoid hard-coding the R version here. Users should be able to select from multiple versions if they have build more than one version of R in the container (as per Multiple R Versions). We could get the which_r script to update the "args" here if necessary.
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 have opened a PR with a demo setup that uses CodeLLDB as I discussed in #196. Perhaps you can try it out to see how I got that working - it is still missing the changes to which_r and requires some manual steps that you currently have in the postCreateCommand.
scripts/launch_r.sh
Outdated
|
|
||
| # If the first argument is a file, use that as R_BINARY; otherwise default. | ||
| if [ -x "$1" ]; then | ||
| R_BINARY="$1" |
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.
Maybe throw an error in this case rather than open a version the user didn't ask for?
But really which_r should handle this: as long as people have defined a valid version of R to switch to, then which_r should set the path correctly here.
4581ad4 to
ecd644a
Compare
modify the step for setting CFLAGS for a better explanation Co-authored-by: Heather Turner <ht@heatherturner.net>
|
Hi @hturner , I have updated this branch in a way that it aligns with your suggestions and comments and should work now logically. Every time you open a new R terminal , it runs the I know this is actually asking a lot but can you please check if this is working on your system once. |
.devcontainer/devcontainer.json
Outdated
| "r.rterm.linux": "/usr/bin/R", | ||
| "terminal.integrated.env.linux": { | ||
| "R_RPATH_LINUX": "${config:r.rpath.linux}" | ||
| }, |
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 think you need this, see below for getting the current version of R.
scripts/launch_r.sh
Outdated
| if [ -n "${R_RPATH_LINUX:-}" ] && [ -x "$R_RPATH_LINUX" ]; then | ||
| R_BINARY="$R_RPATH_LINUX" | ||
| else | ||
| R_BINARY="/usr/bin/R" | ||
| fi | ||
|
|
||
| exec "$R_BINARY" "$@" |
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.
You don't need to do it like this. Define launch_r.sh as
!/bin/bash
export LD_PRELOAD=/workspaces/r-dev-env/scripts/allow_ptrace.so
exec /usr/bin/R "$@"
then update which_r to replace the old path with the new path as required, e.g. (untested)
sed -i "s|^exec .*R|exec $selected_version|" launch_r.sh
This would be a new step in addition to updating "r.rpath.linux" in the VS Code settings.
For initial testing, just change the path manually, so you don't have to get everything working at once!
scripts/localscript.sh
Outdated
|
|
||
| # Remove the .vscode/settings.json file if it exists so that | ||
| # it does not interfere with the devcontainer.json | ||
| rm -f "$WORK_DIR/.vscode/settings.json" |
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.
Do not remove this file, this is the one that is being updated on line 32!
.devcontainer/launch.json
Outdated
| "environment": [ | ||
| { | ||
| "name": "R_RPATH_LINUX", | ||
| "value": "${config:r.rpath.linux}" | ||
| } | ||
| ], |
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.
Should not need this here either. The path to R is only needed when we launch the terminal. The debugger identifies the running R process with the process ID (as returned by Sys.getpid() in R).
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.
Please use the launch.json as I do in #261
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.
Please only change what is necessary in this file: edit line 63 so it only changes r.rpath.linux and add the new call to sed to update launch_r.sh. Otherwise you risk introducing regressions, e.g. line 67 was added (by you!) in #255 to instruct people how to reload the help to use the version corresponding to the R version that has been switched to.
.devcontainer/launch.json
Outdated
| "environment": [ | ||
| { | ||
| "name": "R_RPATH_LINUX", | ||
| "value": "${config:r.rpath.linux}" | ||
| } | ||
| ], |
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.
Please use the launch.json as I do in #261
|
Hi @hturner , |
|
@avinab-neogy I just merged #265 that hides the |
|
Closing this PR as I have opened another PR which is working |
Added lldb debugging functionality
Closes #196 and #30