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
Expose the EdgeDB CLI as npx edgedb
#931
Conversation
packages/driver/src/cli.mts
Outdated
} | ||
|
||
async function downloadCliPackage() { | ||
console.log("No EdgeDB CLI found, downloading CLI package..."); |
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 is the only actual potential output unless you run the command with DEBUG=edgedb
.
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 we only print this if it's running in a terminal? (https://nodejs.org/docs/latest/api/tty.html#tty) If you're running this command in a script, it might be expecting a certain output (particularly if it's using the json flag), which printing this line might break.
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.
Yeah that seems reasonable! Will fix.
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.
Also skipped the console.error
if not TTY. That writes to stderr, so hopefully it was safe before, but seems like a reasonable choice too. Any opinions on that?
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 would have thought that we'd still want to output the error on stderr even in scripts? The command has already failed, and we're returning a non-zero exit code, so to me it seems more useful to output what the error was.
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.
Yeah, that seems fair. There might be some confusion if the edgedb
binary outputs some error text and then we also output some error text here, but I guess we already have that confusion, and otherwise, any of the code in the wrapper wouldn't output anything on errors in non-TTY environments. I'll add it back and we can iterate on this if we get feedback on it being confusing or inconvenient to have to pipe stderr to /dev/null
or something.
return runEdgeDbCli(["_self_install"], pathToCli); | ||
} | ||
|
||
async function findPackage(): Promise<Package> { |
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.
All of this logic is adapted from https://github.com/edgedb/setup-edgedb/blob/main/src/main.ts#L123 and ought to be centralized into a separate package to keep them in sync with each other, but there are some slight differences between the GitHub Actions runtime and a plain Node runtime, so I decided the easiest thing to do for now was to copy.
We can still look at making a common abstraction they can share (probably this logic without the side effects), but I don't want to block this effort on that.
29201dc
to
65ef138
Compare
Changed the naming of this during the implementation in the CLI
Mostly involved confusion between paths to directories and paths to the binary files we are interested in. Also fixes a small issue with a race condition on writing the downloaded temporary CLI, changing the mode of the file, and executing the binary.
Make it more explicit that the temporary CLI might point to an existing binary if it happens to exist at the install directory already. In that case, we don't run the self-install.
65ef138
to
7819a58
Compare
Might revisit this if it's deemed too noisy or unergonomic for script usage.
We wrap the
edgedb
CLI in a Node script that will attempt to call any already installed CLI when runningnpx edgedb [args]
. If it cannot findedgedb
in the currentPATH
, it will download a temporary copy of the CLI and try to get the install directory that the installer would place the binary at. If it finds theedgedb
binary in that install path, it will simply call that binary. If not, it will run the self installer.The case where it will find the
edgedb
binary in the install path, but it is not in thePATH
already is typically right after installing the CLI, since the install path is not yet in the user's shell. It might also be the case that the user is in an unsupported shell, or calling the binary from something with an isolated environment.NOTE: We don't want to ship this in stable
edgedb
npm package while it's pointing atnightly
CLI, so we will land this once the command to get the install path hits stable CLI.Closes #916
I've manually tested the following scenarios from a Docker container:
No
edgedb
in the PATH and no cache file. Downloads the temporary CLI, gets the install directory. Noedgedb
in the install directory, so runs the self-install. Runs the newly installed command with the supplied arguments.No
edgedb
in PATH still, but finds the cache file. Binary is in the cache file directory, so it runs the binary it finds at the cached location.Source the environment file which will set the PATH. The wrapper will now execute the binary directly from the PATH:
Unset the PATH, and remove the cache file. The wrapper will now download the temporary CLI, get the install directory and notice that the binary exists in the install path. It will cache this location, and execute the already installed binary.