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

Expose the EdgeDB CLI as npx edgedb #931

Merged
merged 19 commits into from Apr 5, 2024
Merged

Expose the EdgeDB CLI as npx edgedb #931

merged 19 commits into from Apr 5, 2024

Conversation

scotttrinh
Copy link
Collaborator

@scotttrinh scotttrinh commented Apr 2, 2024

We wrap the edgedb CLI in a Node script that will attempt to call any already installed CLI when running npx edgedb [args]. If it cannot find edgedb in the current PATH, 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 the edgedb 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 the PATH 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 at nightly 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. No edgedb in the install directory, so runs the self-install. Runs the newly installed command with the supplied arguments.

$ npx edgedb --version
No EdgeDB CLI found, downloading CLI package...

Welcome to EdgeDB!

This will install the official EdgeDB command-line tools.

The edgedb binary will be placed in the user bin directory located at:
/root/.local/bin

This path will then be added to your PATH environment variable by modifying the profile file located at:
/root/.profile

┌──────────────────────┬──────────────────┐
│ Installation Path    │ /root/.local/bin │
│ Modify PATH Variable │ yes              │
│ Profile Files        │ /root/.profile   │
└──────────────────────┴──────────────────┘
1) Proceed with installation (default)
2) Customize installation
3) Cancel installation
1
The EdgeDB command-line tool is now installed!

Your shell profile has been updated with /root/.local/bin in your PATH.
It will be configured automatically the next time you open the terminal.

For this session please run:
source "/root/.config/edgedb/env"
To initialize a new project, run:
edgedb project init
EdgeDB CLI 4.1.1+0fdb149

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.

$ npx edgedb --version
EdgeDB CLI 4.1.1+0fdb149

Source the environment file which will set the PATH. The wrapper will now execute the binary directly from the PATH:

$ source "/root/.config/edgedb/env"
$ npx edgedb --version
EdgeDB CLI 4.1.1+0fdb149

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.

$ rm /root/.cache/edgedb-nodejs/cli-location
$ export PATH="path without the install path"
$ npx edgedb --version
No EdgeDB CLI found, downloading CLI package...
EdgeDB CLI 4.1.1+0fdb149

}

async function downloadCliPackage() {
console.log("No EdgeDB CLI found, downloading CLI package...");
Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaclarkee873d0f (#931)

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?

Copy link
Member

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.

Copy link
Collaborator Author

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> {
Copy link
Collaborator Author

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.

@scotttrinh scotttrinh force-pushed the 916-edgedb-cli branch 2 times, most recently from 29201dc to 65ef138 Compare April 3, 2024 13:12
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.
@scotttrinh scotttrinh merged commit fc6ad27 into master Apr 5, 2024
9 checks passed
@scotttrinh scotttrinh deleted the 916-edgedb-cli branch April 5, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add npx edgedb binary command that forwards commands to EdgeDB CLI and installs the CLI if it's missing
2 participants