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

Extend the "describe" op with some extra runtime information #181

Open
bbatsov opened this issue Apr 5, 2020 · 24 comments
Open

Extend the "describe" op with some extra runtime information #181

bbatsov opened this issue Apr 5, 2020 · 24 comments

Comments

@bbatsov
Copy link
Contributor

bbatsov commented Apr 5, 2020

Is your feature request related to a problem? Please describe.

Today it's hard to tell what's the runtime of the server you're connecting to (e.g. Clojure, ClojureScript, ClojureCLR, babashka, or some non-Clojure language like Racket, fennel, etc.).

One practical problem is that client often implement some functionality in terms of evaluating runtime-specific code, which often means that one client can't be used with different nREPL servers simply because it makes some assumptions that are not universally true.

If clients could easily infer that runtime of the server they can support easily different runtimes.

Describe the solution you'd like

I think we should agree that every nREPL server should return some extra metadata in the describe op:

  • runtime/language (e.g. Clojure)
  • protocol version (tracking the version number of the reference nREPL implementation) - not sure how useful this can be, but it won't hurt. I guess potentially this can be nrepl-version.
  • name of the server implementation (e.g. Ogion)
  • version of the server implementation (this should be different from nrepl-version I guess)

E.g. you can have something like:

{:runtime "racket"
  :protocol-version "0.7"
  :implementation-name "ogion"
  :implementation-version "0.1.0"}

Describe alternatives you've considered

I didn't manage to think of a good alternative.

@nrepl/nrepl @technomancy @borkdude setzer22 Let me know how this sounds to you.

@borkdude
Copy link

borkdude commented Apr 5, 2020

In the case of babashka, should the runtime be "clojure" or "babashka"? Babashka mimics JVM Clojure as close as possible, but some things aren't supported like defprotocol. I'm not sure what to put as the nREPL protocol version. I just use bencode and reverse engineered what was necessary to make CIDER's cider-connect and lein repl :connect work.

{:runtime "babashka" ;; or "clojure" ?
  :protocol-version "??"
  :implementation-name "??"
  :implementation-version "??"}

@technomancy
Copy link
Collaborator

technomancy commented Apr 5, 2020 via email

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

I finally started working on this today, so I'll have some update soon. Now I'm leaning towards adding a runtime and name/server (short for "implementation/server name") key at the top-level of the describe map. Servers will still put their relevant versions in the versions map, but clients would know what to look for based on the runtime/server keys (e.g. if runtime is clojure/babashka they'll look for clojure/babashka and java version info). If in the case of fennel they'll look for fennel and lua version info and so on.

@borkdude In your case runtime should be "babaska", "name" whatever you want (e.g. nrepl-babashka) and same with the version:

{:runtime "babashka"
  :name "nrepl-babashka"
  :versions { :babashka "..."
                     :java "..."
                     :nrepl-babashka "..."}}

Let me know how this sounds to you and whether you have some preferences about the names of the extra keys.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

Btw, I guess for babashka the runtime can easily be clojure as well, given how highly compatible they are. That's not a big deal in the end of the day I think.

@borkdude
Copy link

borkdude commented Jun 2, 2020

Having a separate version / java from version / babashka doesn't make sense, since you cannot have different Java/GraalVM versions with the same babashka version, i.e it's a derived property. From 0.1.0 on babashka is compiled with GraalVM java11 (right now GraalVM version 20.1.0).

I think the field "name" could have a more descriptive name. What is this going to be used for? In the case of babashka there is only one possibility: [babashka.nrepl]

To summarize, I think for babashka this makes sense:

{:runtime "babashka"
 :versions {:babashka "0.1.0"}}

and that's about it?

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

Having a separate version / java from version / babashka doesn't make sense, since you cannot have different Java/GraalVM versions with the same babashka version, i.e it's a derived property. From 0.1.0 on babashka is compiled with GraalVM java11 (right now GraalVM version 20.1.0).

Got it. Well, one less version then.

I think the field "name" could have a more descriptive name. What is this going to be used for? In the case of babashka there is only one possibility: [babashka.nrepl]

For most runtimes there's just one server currently, so potentially we can skip this. There's no guarantee there won't be more servers down the road, but there's also no real need for premature optimization at this point.

and that's about it?

Fine by me.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

Btw, doesn't your nREPL server have different versioning from babashka - I guess it still might be nice to include its version so that clients could display it.

@borkdude
Copy link

borkdude commented Jun 2, 2020

@bbatsov True that. We could add :babashka.nrepl "0.0.4" to versions?
https://github.com/babashka/babashka.nrepl/blob/c3bfaff12c84d269e0d1c6110c290d5c5a08e002/project.clj#L1

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

We have a deal! 🤝

@borkdude
Copy link

borkdude commented Jun 2, 2020

@bbatsov There are other projects that use babashka.nrepl as their nREPL server. Their runtime, like babashka, is based on the sci interpreter. I made a proposal here for these projects:

babashka/babashka.nrepl#17

Is this information going to be used by CIDER? Would it be useful to have one common key so CIDER doesn't have to add cases for each new sci-based project? It's not always clear what a sci-based project will or won't support though, since sci is just an interpreter. What it can provide depends on how people use the interpreter. So it's likely that an nREPL client will still have to try things with resolve etc. to determine what the nREPL server can do.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

Is this information going to be used by CIDER?

Yeah, it will be. The plan is to have CIDER support better non-Clojure nREPL servers See clojure-emacs/cider#2848 for more details. As an immediate step for babashka I wanted to make some changes that would eliminate the warnings they are getting from CIDER today. Unlike a language like Fennel it doesn't really require much internal changes, as most of the Clojure code that CIDER runs will work just fine.

So it's likely that an nREPL client will still have to try things with resolve etc. to determine what the nREPL server can do.

Well, if the runtime is not Clojure-like we can't even try resolve. :-) That's why the runtime info is the most important one.

@borkdude
Copy link

borkdude commented Jun 2, 2020

@bbatsov If that's the goal, maybe for runtime it's better to use some namespaced thing, so the project can describe it's some flavor of Clojure?

:runtime "clojure/babashka"
:runtime "clojure/bootleg"
:runtime "clojure/clojurescript"

So then CIDER knows by the namespace of the key that it can use require?

Or a separate :language key that says "clojure".

@plexus
Copy link
Contributor

plexus commented Jun 2, 2020

I've been following this conversation with some trepidation, as it reminds me of how the web has struggled to improve because of everyone doing user agent checks. Instead of checking for implementation/version it would be more robust and forward thinking to announce and check for specific capabilities...

@borkdude
Copy link

borkdude commented Jun 2, 2020

@plexus Good point. That's exactly why I have avoided communicating a pod version in babashka.pods but so far have only communicated capabilities using the :ops op which is also part of nREPL.

Stating a :language "clojure" describes it's capable of something although you still don't know if the implementation of clojure is complete.

Btw, this was weird:

Screenshot 2020-06-02 11 36 11

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

Yeah, maybe we should have a separate key about the language, as with ClojureScript you can have both a Java runtime and a self-hosted JavaScript runtime and you need to be able to differentiate between the two. On the other hand from what I gather some servers like Ogion can handled two languages (Lua and Fennel), even if the server is implemented in Lua and obviously Lua is the actual runtime (as it's Java for regular Clojure). With hosted languages we have two layers of runtime which makes things a bit more complicated.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

I've been following this conversation with some trepidation, as it reminds me of how the web has struggled to improve because of everyone doing user agent checks. Instead of checking for implementation/version it would be more robust and forward thinking to announce and check for specific capabilities...

The problem is that many clients patch some missing capabilities by evaluating code and they need to know what exactly to evaluate. If we remove the evaluations from the equation we don't really care about the runtime. If we need to auto-detect a connection type some extra info would be useful as well. But now I'm wondering if I'm not approaching this from the wrong angle. :-)

Stating a :language "clojure" describes it's capable of something although you still don't know if the implementation of clojure is complete.

Yeah, this might work as well.

@borkdude
Copy link

borkdude commented Jun 2, 2020

But now I'm wondering if I'm not approaching this from the wrong angle. :-)

Maybe good to take a step back. Start with a clear problem statement. Then explore alternative solutions. Then choose one minimal solution that could be extended later if necessary.

To restate the problem: CIDER (or other editor) is missing information about the nREPL server. It could try resolve to gain this information, but resolve might not even work. Is that a problem? If the nREPL server sends back an exception on resolve, why not try the next thing to try if the server speaks Fennel or some other language? That might be painful. To relieve this pain, only adding a :language key might be sufficient for now, although there is no guarantee that if an nREPL server says it speaks clojure, that it also supports resolve? At least it narrows down the amount of things to try next. What are the alternatives?

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

  1. The target language gets set manually in clients (e.g. on connect)
  2. Clients restrain themselves from implementing any functionality in terms of eval and just rely on nREPL ops for everything

Probably there are other options, but those two are the obvious ones.

@borkdude
Copy link

borkdude commented Jun 2, 2020

Can you elaborate on 2? Not sure I understand.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 2, 2020

E.g. you want to provide code completion by just evaluating code in some code completion library (as opposed to a dedicated op) - that's how Leiningen currently provides code completion in its REPL. That's a common approach many people take, as it's the simplest thing to do in many cases and it's the main issue when it comes to clients portability.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 3, 2020

Btw, there's one more nuance we have to keep in mind - how do you probe for capabilities that are not actually ops (e.g. the print middleware doesn't expose any ops, as it just modifies requests and responses). We can have them somewhere in describe I guess, e.g. under some capabilities key or something.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jun 12, 2020

Seems the conversation here got stalled, so I won't change anything for time being. @borkdude can just add this to babashka.nrepl:

:versions {:babashka "0.1.0"
                  :babashka.nrepl "..."}

So I can teach CIDER about it. I played a bit with babashka and CIDER today and all the nil versions and warnings on startup are something I'd like to fix.

@borkdude
Copy link

Will do!

@technomancy
Copy link
Collaborator

Clients restrain themselves from implementing any functionality in terms of eval and just rely on nREPL ops for everything

For the record I'm 100% in favor of this; at the very least implementing functionality using eval should be considered a measure of last resort that you fall back to only once you determine that the op you need is not present.

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

No branches or pull requests

4 participants