-
Notifications
You must be signed in to change notification settings - Fork 4.4k
[codex-cli] Add ripgrep as a dependency for node environment #2237
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
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 imperative that you test this with:
./scripts/publish_to_npm.py --dry-run
and make sure all the files that you expect to go in the .tgz
end up there.
I believe this is responsible for ensuring which files are included:
Lines 12 to 15 in eaa3969
"files": [ | |
"bin", | |
"dist" | |
], |
I don't think there are any new files, but to be sure, I would use ./scripts/publish_to_npm.py --dry-run
to create the .tgz
and then do a local install using that to ensure that the dependencies
get installed into node_modules
as expected and everything lines up.
codex-cli/bin/codex.js
Outdated
} | ||
} | ||
|
||
async function resolveRgPath() { |
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.
In deleting most of codex-cli
, I guess we lost Prettier coverage for this file...
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.
Updated package.json scripts to include js files!
package.json
Outdated
"packageManager": "[email protected]" | ||
"packageManager": "[email protected]", | ||
"dependencies": { | ||
"@vscode/ripgrep": "^1.15.14" |
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.
Note that this does not guarantee that rg
gets installed because the npm package does not bundle the binaries itself, but downloads them via postinstall
:
https://www.npmjs.com/package/@vscode/ripgrep
Because we expect our users to install via npm -g
and have network access at that time, this should be fine, but if they used --ignore-scripts
for some reason, then it wouldn't end up getting fetched.
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.
Yep, agree it's not bullet-proof! However, the postinstall script also resolves to a platform-specific build for us, which is a nice property. I think the tradeoff is worth it. We could add a note to one of our docs about installing correctly
Tested with the following steps: # on local branch
npm pack --pack-destination ../dist
scp ./dist/openai-codex-0.0.0-dev.tgz user@devbox:~/code/test/
# on devbox
tar -xzf openai-codex-0.0.0-dev.tgz
cd package
npm install
./bin/codex.js e --skip-git-repo-check "run which rg. do not use bash -lc"
...
rg found at: `/root/code/test_codex_npm/package/node_modules/@vscode/ripgrep/bin/rg` |
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.
Everything looks good, but I would make it so the CLI still runs if we can't load rg
from @vscode/ripgrep
.
Summary
Ripgrep is our preferred tool for file search. When users install via
brew install codex
, it's automatically installed as a dependency. We want to ensure that users running via an npm install also have this tool! Microsoft has already solved this problem for VS Code - let's not reinvent the wheel.This approach of appending to the PATH directly might be a bit heavy-handed, but feels reasonably robust to a variety of environment concerns. Open to thoughts on better approaches here!
Testing
node -e "const { rgPath } = require('@vscode/ripgrep'); require('child_process').spawn(rgPath, ['--version'], { stdio: 'inherit' })"
rg
uninstalled, asked it to runwhich rg
. Output below: