A port of the nbb nrepl-server to basilisp

[ikappaki]

Hi,

could you please review patch to port nbb's build in nrepl-server to basilisp contrib. It addresses #412.

nbb has the same EPL-1.0 license as basilisp so I believe using its code as the base for this PR should not cause licensing issues.

The nbb bencode module which is a dependency was ported as well. Extensive tests were written for both.

The server has been tested to work with both CIDER and Calva in VS-Code.

The server can be invoked with baslisp nrepl-server:

$ poetry run basilisp nrepl-server -h
usage: basilisp.cmd nrepl-server [-h] [--host HOST] [--port PORT] [--port-filepath PORT_FILEPATH]

Start the nREPL server.

optional arguments:
  -h, --help            show this help message and exit
  --host HOST           the interface address to bind to, defaults to 127.0.0.1.
  --port PORT           the port to connect to, defaults to 0 (random available port).
  --port-filepath PORT_FILEPATH
                        the file path where the server port number is output to, defaults to ".nrepl-port".

Several fixes were required to make it work, which are included as separate commits in this PR prior to the main bencode and nrepl-server commits. They address the following issues

• Fixed issue with sort-* fns returning an error on empty seqs (sort-* function fails on empty seq #716)
• Fix issue with ns being unavail after in-ns during eval (Unable to setup namespace with ns after its creation using in-ns #718)
• Fixed issue with import modules aliasing using ns eval (import does not work while evaling an ns #719)
• Fixed issue with ns-resolve throwing error on macros (error raised when trying to ns-resolve a macro #720)

support for the basilisp.stacktrace/print-cause-trace is also added because it is requested by CIDER when an exception is thrown to show to the user, which is essential, it partially addresses #721.

In addition, two issues were observed while running the tests in CI

• The 1.6.0 was released few days ago and complains for an additional type error in the generator, which is now ignored with this patch

Traceback (most recent call last):
#...
py311-mypy: commands[0]> mypy --config-file=C:\src\basilisp/pyproject.toml -p basilisp
src\basilisp\lang\compiler\generator.py:3530: error: Argument 1 to "fields" has incompatible type "type[IType]"; expected an attrs class  [misc]
src\basilisp\lang\compiler\generator.py:3530: note: Error code "misc" not covered by "type: ignore" comment

• The nrepl-server tests which are using threads were failing due to trying to release an rlock which has been already released whose essence is captured in potential issue with readerwritelocks usage in basilisp core libs #722, and is fixed with this patch as described in the ticket by using with self._lock.gen_rlock() instead of a single instance of with self._rlock(). The error was

    File "/home/runner/work/basilisp/basilisp/.tox/py39/lib/python3.9/site-packages/basilisp/lang/runtime.py", line 706, in find
    return v
   File "/home/runner/work/basilisp/basilisp/.tox/py39/lib/python3.9/site-packages/readerwriterlock/rwlock.py", line 49, in __exit__
    self.release()
   File "/home/runner/work/basilisp/basilisp/.tox/py39/lib/python3.9/site-packages/readerwriterlock/rwlock.py", line 344, in release
    if not self.v_locked: raise RELEASE_ERR_CLS(RELEASE_ERR_MSG)
 RuntimeError: release unlocked lock

I understand this could be a lot to go through and there likely to be multiple iterations while I update the code with your feedback, of which I'm looking forward to.

I have not written a nrepl-server section for the manual yet. I plan to do this after a successful review, and will also update the changelog.

Thanks

[ikappaki]

Rebased from main to resolve conflicts and updated changelog with issue numbers.

[chrisrink10]

This is definitely very cool and I'm excited to see how this works.

Would it be possible to start breaking off the smaller bug fixes as individual PRs so this PR is just the nrepl server? Most of them seem straightforward enough on their own.

My one reservation with using code from another project is that I assume we'll need to correctly attribute the source of that code to its original author(s) and I'm not 100% sure how to do that. Perhaps a copyright notice at the top of the file attributing that author and mentioning the license is the same as the main project.

[ikappaki]

Would it be possible to start breaking off the smaller bug fixes as individual PRs so this PR is just the nrepl server? Most of them seem straightforward enough on their own.

done, merged to main and rebased this branch, ready for review.

My one reservation with using code from another project is that I assume we'll need to correctly attribute the source of that code to its original author(s) and I'm not 100% sure how to do that. Perhaps a copyright notice at the top of the file attributing that author and mentioning the license is the same as the main project.

Indeed we should, I've already chatted with the author about it and plan to bring them in to discuss about it once we pass the review and have a final product (include documentation).

[chrisrink10]

I'm still working my way through this but I had a few comments and questions to start with.

Thanks again for putting this together! This is extremely cool and I'm so excited it's happening.

[chrisrink10]

It might be nice if we could split the bencode implementation off as a separate PR once we resolve the couple of comments I made there.

[chrisrink10]

It might be nice if we could split the bencode implementation off as a separate PR once we resolve the couple of comments I made there.

Though I also realize this may make the discussions between us and the nbb folks more complicated, so maybe not.

[ikappaki]

It might be nice if we could split the bencode implementation off as a separate PR once we resolve the couple of comments I made there.

Though I also realize this may make the discussions between us and the nbb folks more complicated, so maybe not.

I don't foresee any objections to assigning credits; I expect the discussion to focus more on deciding where to allocate them.

I consider the bencode package necessary solely for supporting the nrepl-server and not as a standalone general-use package, although it could be viewed differently. Personally, I prefer not to invest an extensive amount of time on it. Initially, I attempted to utilize https://pypi.org/project/bencode/, but unfortunately, it lacks an API to retrieve the undecoded portion of the stream, making it unsuitable for this specific nREPL server implementation.

Thanks

[chrisrink10]

Sorry for the delay. Finally took a first pass on the nrepl server itself.

[ikappaki]

Hi, I've updated the documentation to include references to the nREPL server.

It also includes two autogen fixes

1. the get_object_members fn was missing from the VarDocumenter, a dummy implementation was introduced
2. The api generator failed to locate the files whose namespace included hypens (such as nrepl-server).

Thanks

[ikappaki]

(Updated to pick up latest changes from main and adjusted code accordingly)

[ikappaki]

merged replacement bencode lib from main

[chrisrink10]

I finally took a pass on the tests. Really nice, comprehensive suite. Thanks for doing that.

[chrisrink10]

I have no further comments for now.

Is there anything else we need to do to discuss with nbb's authors on our use of their tooling or are our current attributions sufficient?

[ikappaki]

I have no further comments for now.

Is there anything else we need to do to discuss with nbb's authors on our use of their tooling or are our current attributions sufficient?

Hi @borkdude, as mentioned briefly the other day, I've ported the nbb nREPL server (and together with @chrisrink10 the bencode library as a dependency) to basilisp. I'm seeking suggestions, if any, on how to appropriately attribute copyright and credits to both the nbb project and yourself. Besides adhering to the EPL 1.0 license for basilisp, acknowledging nbb in comments and documentation, and including a comment at the top of the files referencing the specific GitHub commit where the initial development originated, do you have any additional recommendations? Thanks

[borkdude]

Seems sufficient to me. I too borrowed the initial code from some other project. Standing on shoulders, etc :)

[ikappaki]

Seems sufficient to me. I too borrowed the initial code from some other project. Standing on shoulders, etc :)

Thanks a lot for all your hard work!