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

do not include $FILENAME in globals #1105

Merged
merged 1 commit into from
May 30, 2024
Merged

do not include $FILENAME in globals #1105

merged 1 commit into from
May 30, 2024

Conversation

nyz93
Copy link
Contributor

@nyz93 nyz93 commented May 30, 2024

$FILENAME is unsafe to include in the globals because it modifies ARGV and raises an error if it's a file that does not exist. This causes two issues if rdbg used to debug a command with arguments:

  • when using rdbg the server will crash when running info globals, as it does not catch the error
  • the DAP server catches this error, so when using a DAP client that requests globals before the debugged program was able to handle its arguments then the ARGV changes and the program will not do what's expected

`$FILENAME` is unsafe to include in the globals because it modifies ARGV
and raises an error if it's a file that does not exist. This causes two
issues if rdbg used to debug a command with arguments.
 * when using rdbg the server will crash when running `info globals`, as
   it does not catch the error
 * when using a DAP client that requests globals before the debugged
   program was able to handle its arguments then the ARGV changes and
   the program will not do what's expected
@ko1 ko1 merged commit 8abc50a into ruby:master May 30, 2024
5 checks passed
@ko1
Copy link
Collaborator

ko1 commented May 30, 2024

Thank you!

Copy link

launchable-app bot commented May 30, 2024

Launchable Report

❌ Test session #2946090 failedos:ubuntu-latest test_task:test_protocoldetails on CI
🔔 no issues ✖️1 test failed ✔️63 tests passed

❌ Test session #2946091 failedos:ubuntu-latest test_task:test_protocoldetails on CI
🔔 no issues ✖️1 test failed ✔️63 tests passed

❌ Test session #2946093 failedos:ubuntu-latest test_task:test_protocoldetails on CI
🔔 no issues ✖️1 test failed ✔️63 tests passed

❌ Test session #2946094 failedos:ubuntu-latest test_task:test_protocoldetails on CI
🔔 no issues ✖️1 test failed ✔️63 tests passed

❌ Test session #2946120 failedos:ubuntu-latest test_task:test_consoledetails on CI
🔔 no issues ✖️1 test failed ✔️296 tests passed

Passed test sessions

✅ Test session #2946072 passed os:ubuntu-latest test_task:test_testdetails on CI
✅ Test session #2946073 passed os:ubuntu-latest test_task:test_testdetails on CI
✅ Test session #2946076 passed os:ubuntu-latest test_task:test_testdetails on CI
✅ Test session #2946077 passed os:ubuntu-latest test_task:test_testdetails on CI
✅ Test session #2946080 passed os:ubuntu-latest test_task:test_testdetails on CI
✅ Test session #2946082 passed os:ubuntu-latest test_task:test_testdetails on CI
✅ Test session #2946084 passed os:ubuntu-latest test_task:test_protocoldetails on CI
✅ Test session #2946085 passed os:ubuntu-latest test_task:test_protocoldetails on CI
✅ Test session #2946096 passed os:ubuntu-latest test_task:test_protocoldetails on CI
✅ Test session #2946103 passed os:macos-latest test_task:test_consoledetails on CI
✅ Test session #2946104 passed os:ubuntu-latest test_task:test_consoledetails on CI
✅ Test session #2946106 passed os:ubuntu-latest test_task:test_consoledetails on CI
✅ Test session #2946107 passed os:macos-latest test_task:test_consoledetails on CI
✅ Test session #2946111 passed os:ubuntu-latest test_task:test_consoledetails on CI
✅ Test session #2946114 passed os:macos-latest test_task:test_consoledetails on CI
✅ Test session #2946121 passed os:ubuntu-latest test_task:test_consoledetails on CI
✅ Test session #2946123 passed os:ubuntu-latest test_task:test_consoledetails on CI
✅ Test session #2946141 passed os:ubuntu-latest test_task:test_consoledetails on CI

Build: refs_pull_1105_merge_bb889708c935cc45b725629ceadecf0b2666927f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants