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

FR: JJ extension suppport #3575

Open
matts1 opened this issue Apr 26, 2024 · 8 comments
Open

FR: JJ extension suppport #3575

matts1 opened this issue Apr 26, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@matts1
Copy link
Contributor

matts1 commented Apr 26, 2024

Is your feature request related to a problem? Please describe.
I'm trying to make a jj gerrit subcommand which will support gerrit. I imagine that people will make commands like jj github, jj lint, jj fix.

Describe the solution you'd like
Not so much what I'd like - I'm not particularly attached to the idea and welcome other ideas, but my thoughts were:

  • I could add the following to my jj config.toml:
[extensions]
gerrit = "/path/to/gerrit/binary"
other_ext = { binary = "/path/to/other/binary", server_opts=... }

Running jj gerrit foo bar would:

  1. Recognize that the command doesn't exist, but it's an extension
  2. Create a pipe
  3. Start up a jj grpc server for the current jj process, connect it to the pipe
  4. Start the subproccess /path/to/gerrit/binary gerrit foo bar with the environment variables JJ_SERVER=fd:3

The subprocess would then:

  1. Read JJ_SERVER to get the specifications for the grpc channel and create a grpc connection over these.
  2. Communicate with the jj parent process via grpc
  3. Exit, which the parent process would see, and then kill the grpc server.

Describe alternatives you've considered

  • I considered communicating over stdin / stdout, but neither was viable
  • Putting gerrit in the jj binary itself
    • Could have a fork, but that means that multiple extensions at once is a nightmare
    • Probably not appropriate for everyone
    • Could lock it behind a feature flag, but still probably shouldn't be in the repo, and any extension would need to be approved by jj itself
  • Could invoke jj_lib yourself, but then:
    • I imagine that an older version of jj_lib might cause corruption of the VCS directory
    • Doesn't work very well for cross-language support

Additional context
Add any other context or screenshots about the feature request here.

@thoughtpolice
Copy link
Member

thoughtpolice commented Apr 26, 2024

Note that I am adding Gerrit support in #2845; it has some rough edges still, but I (and several of my colleagues) use it almost every day. I mostly haven't done another round of review yet because I still have 1 or 2 things left to do before the PR is complete (notably adding tests, and documentation for users). I rebase it regularly. Nobody is particularly opposed to it I think, and I believe there is still much interest in adding other specific forge integrations into jj directly (github, etc.) Realistically in the case of forge support, I think having "First-class" support directly is nicer than the typical status quo in the Git world, which is "Everyone just finds some random binary and throws it into their workflow and they all work somewhat differently." That said, it's much more work for us. I suspect we'd probably only ever realistically have a ~handful of integrations, too.

That said, I have no particular argument against the actual FR here, which is for out of tree extensions that can sit somewhere in the command namespace.

Actually, as far as the specific design you suggest, I think exposing most of the jj's existing interfaces over RPC to arbitrary clients is, broadly speaking, an excellent goal in the long run because it not only paves a way for your extensions example — it also solves many of the thornier issues with things like dynamic loading and distributing custom binaries that are built in jj_cli. IMO, an RPC interface is actually vastly more important and broadly applicable than just being able to add subcommands to jj. For example, VSCode integration would work for any version of jj the user has, even if it has many extra additions, like custom backends.

Related issue: #3001 — there has also been much more discussion about this over time in various places like Discord, so others should definitely chime in with their thoughts.

@martinvonz
Copy link
Member

#3034 is also related

@matts1
Copy link
Contributor Author

matts1 commented Apr 26, 2024

Ah, so the alternative suggested in #3034 would be to define jj-foo, similar to how git does it. I'm personally not in favor of that, since I believe that will just lead to binaries invoking jj commands themselves directly via the CLI. How do other people feel about it?

I quite like the fact that with my idea, jj has already started a gRPC server for you, scoped to that specific invocation. This means you don't need to worry about making sure that a long-running daemon is always running, or invoking jj api many times.

I think we could also provide optional language support, for example:

foo_extension = { binary = "/path/to/foo_ext.py", prelude = "python" }

And then jj itself would invoke a binary that does something along the lines of (pseudocode):

conn = grpc.connect(file_descriptor(os.environ["JJ_SERVER"]))
main = importlib.import("/path/to/foo_ext.py", "main")
main(conn, sys.argv)

This would significantly reduce the barrier to entry for an extension, meaning that all you need to do is:

def main(conn, argv):
    # Start using conn

@PhilipMetzger
Copy link
Contributor

There's also #3219 which is along the same line and should be developed along with this.

@PhilipMetzger PhilipMetzger added the enhancement New feature or request label Apr 26, 2024
@noahmayr
Copy link
Contributor

Imo we would ideally be able to use extensions for backend implementations as well, maybe one of these crates can help with that:
Maybe https://github.com/ZettaScaleLabs/stabby or https://github.com/extism/extism

Stabby (a stable rust ABI create) would mean that extensions could only be written in Rust though, while extism would be an embedded wasm runtime which supports a handful of plugin languages. If lua is sufficient as a plugin language, https://github.com/mlua-rs/mlua could also be an option

@martinvonz
Copy link
Member

If we do need backends to be added without recompiling the binary, then maybe it would be enough to have a single proxy backend that implements the Backend trait and delegates to a configured gRPC address. I think we can wait and decide about that when we have some real use cases.

@thoughtpolice
Copy link
Member

Imo we would ideally be able to use extensions for backend implementations as well, maybe one of these crates can help with that:
Maybe https://github.com/ZettaScaleLabs/stabby or https://github.com/extism/extism

This isn't just about new backends, but — I am strongly against an approach leaning on the dynamic linker (stabby) unless we survey the use cases and come to the conclusion that it's absolutely necessary; it brings a very long tail of complications and many supposed benefits do not always materialize in practice on all platforms. extism is more interesting, but I think we'd have to do a lot of work for anything except Rust to be usable anyway (for one, we basically need to write FFI bindings for the jj hostcall APIs for N languages, in order to have anything that is reasonably usable OOTB), and the host<->sandbox bridge implies a lot of other technicalities that are not easy to work around without writing your own crates, anyway. For example, it would probably not be possible to use extism to, say, write a new working copy implementation that used something like FUSE, because the FUSE library would need to be compiled into the host application anyway before you could reasonably expose APIs for it. You end up back at square 1.

Realistically I think adding backends/trait impls is far, far less of a pertinent issue for users, so we should only ever cross that bridge when (if) we get to it and there are no other options.

@senekor
Copy link
Contributor

senekor commented Nov 4, 2024

I'm reading through this discussion from the perspective of wanting to convice people that jj util exec is a good idea. (discussion: #3001, implemention: #4759)

I noticed that jj util exec is basically a subset of the original idea here. It invokes another program, but doesn't expose a gRPC server to it. That is an extension that could later be added. If the gRPC server is cheap to spin up, we could do it for every invocation, even if most people would just use it for little scripts. If that's too expensive, we could add a flag to jj util exec that enabled the gRPC server feature. The original use case could then be solved along the following lines:

[aliases]
gerrit = ["util", "exec", "--with-grpc-server", "--", "/path/to/gerrit/binary"]
#                         ^^^^^^^^^^^^^^^^^^^^
#                    optional, to be discussed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants