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

Jack in support for the Basilisp language #2560

Merged

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Jun 2, 2024

What has changed?

  • Jack in support for the Basilisp programming language. Tested on Linux, MS-Windows and MacOs, also added an integration test. Added documentation. I followed nbb as an example, thus the new entries are in close proximity with each other. I've also introduced a new baslispPath config option.
  • Fixed a bug in project-root.ts to account for platform dependent path separator, discovered while running the integration tests on MS-Windows.

Fixes #2559

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
  • Made sure there is an issue registered with a clear problem statement that this PR addresses, (created the issue if it was not present).
    • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • Added to or updated docs in this branch, if appropriate
  • Tests
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
  • Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
  • Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik

Copy link

netlify bot commented Jun 2, 2024

Deploy Preview for calva-docs ready!

Name Link
🔨 Latest commit c2a170d
🔍 Latest deploy log https://app.netlify.com/sites/calva-docs/deploys/665c9d2aeda1e10008cd40c5
😎 Deploy Preview https://deploy-preview-2560--calva-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ikappaki ikappaki force-pushed the feature/basilisp-jack-in-support branch from 42bf173 to a59009e Compare June 2, 2024 13:46
@ikappaki ikappaki force-pushed the feature/basilisp-jack-in-support branch from a59009e to b611d70 Compare June 2, 2024 13:52
@PEZ
Copy link
Collaborator

PEZ commented Jun 2, 2024

Hi! Thanks for helping with Calva! 🙏

I'm not sure what it takes to test this. Can you provide some instructions for someone that knows nothing at all about Basilisp and whose Python experience is from decades ago?

It's great with an integration test, but it adds a dependency requirement to anyone who runs the tests, and I don't think we can allow us to do that. So I think the test needs to be resilient on Python and/or Basilisp missing and not run the test in that case. For CI we could consider installing it, if it doesn't add too much time to the runs.

@PEZ
Copy link
Collaborator

PEZ commented Jun 2, 2024

For the basilisp path config: Do we need it? We do not have configs for clojure or lein paths for instance. Instead we require that those are on the user's PATH.

@ikappaki
Copy link
Contributor Author

ikappaki commented Jun 2, 2024

Hi! Thanks for helping with Calva! 🙏

I'm not sure what it takes to test this. Can you provide some instructions for someone that knows nothing at all about Basilisp and whose Python experience is from decades ago?

HI @PEZ,

To test locally, you need to have Python versions 3.8 to 3.12 installed. Use the Python package manager pip to install Basilisp. Once installed, you can start the REPL or nREPL server from the command line. After that, everything is Clojure, and no additional Python knowledge is required except for interoperability with the Python runtime. For more info see the Basilisp Documentation, and some example notebooks that showcase interop with Python.

$ <install python>
$ pip install bassilisp 
# ...
$ basilisp repl 
basilisp.user=> (require '[clojure.string :as str])
nil
basilisp.user=> (str/replace "abc" "b" "x")
"axc"
^C
$ basilisp nrepl-server
nREPL server started on port 49640 on host 127.0.0.1 - nrepl://127.0.0.1:49640

Basilisp code files use the .lpy extension. To "jack in" to a project with Calva, ensure that there is an empty basilisp.edn file at the root of the project (similar to deps.edn).

It's great with an integration test, but it adds a dependency requirement to anyone who runs the tests, and I don't think we can allow us to do that. So I think the test needs to be resilient on Python and/or Basilisp missing and not run the test in that case. For CI we could consider installing it, if it doesn't add too much time to the runs.

I'll make the test conditional on the basilisp executable being in the PATH or running on CI. The CI reports that it takes 20 seconds to install Python and Basilisp, and another 14 seconds to run the jack-in test, which I think is acceptable for integration testing. Running the Basilisp jack-in test is very fast, but it requires compiling the Python files the first time it is executed.

thanks

@PEZ
Copy link
Collaborator

PEZ commented Jun 2, 2024

I have now tested both jack-in and connect. Works like a charm.

@ikappaki
Copy link
Contributor Author

ikappaki commented Jun 2, 2024

For the basilisp path config: Do we need it? We do not have configs for clojure or lein paths for instance. Instead we require that those are on the user's PATH.

Unfortunately, I can't think of better way to support Python virtual environments. Python uses virtual environments to create isolated directories containing pristine copies of a Python installation. For example, you can create a virtual environment in a directory like a/path/to/virtenv, activate the virtual environment, and then install Basilisp there. The directory where Basilisp is installed within the virtual environment is not in the global PATH. If we need to refer to this installation from Calva (to choose the virtual environment), it can be done via a configuration option.

It's very common to have one or more Python virtual environments installed, and we need a way to specify which one to use in Calva for Basilisp.

Another idea is to use the VS Code Python extension to let the user choose the virtual environment they want. The extension provides a command to select the Python interpreter to the virtual environment. However, this approach requires the Python extension to be installed and for Calva to interface with it. I initially tried this method but couldn't successfully retrieve the value—it’s not straightforward to do, and could be confusing having the user to use that extension.

@PEZ
Copy link
Collaborator

PEZ commented Jun 2, 2024

Let's go with the basilispPath config then.

And let's try with adding the integration test to CI. If we deem it to take too much time from the feedback-loop we can remove it later.

Another thing. In the test-data directory there are some projects for different scenarios. You could add a minimal-basilisk project there and make it as minimal as just an empty basilisk.edn, or a tad less minimal than that. This way it is straightforward to make things like sanity checks of the integration (jack-in/connect support).

- Do not fail test if executable is not found outside of CircleCI
- Create minimal-basilisp project for testing
@ikappaki
Copy link
Contributor Author

ikappaki commented Jun 2, 2024

And let's try with adding the integration test to CI. If we deem it to take too much time from the feedback-loop we can remove it later.

Another thing. In the test-data directory there are some projects for different scenarios. You could add a minimal-basilisk project there and make it as minimal as just an empty basilisk.edn, or a tad less minimal than that. This way it is straightforward to make things like sanity checks of the integration (jack-in/connect support).

I've tried to address review comments with latest commits, extract from CI:

Integration testing, Jack-in: start repl and connect (jack-in) to Basilisp
Integration testing, Jack-in: Basilisp executable found at /home/circleci/.local/bin/basilisp
Settings written to /home/circleci/calva/test-data/.vscode/settings.json
[2650:0602/163133.308061:ERROR:bus.cc(407)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")
[2650:0602/163133.524281:ERROR:bus.cc(407)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")
Integration testing, Jack-in: ../projects/minimal-basilisp/src/test.lpy opened for project type basilisp
Jack-in process kill requested
Jack-in terminal killProcess(): Closing any ongoing stdin event
Jack-in terminal killProcess(): Killing process using tree-kill
Jack-in process kill requested
Jack-in terminal killProcess(): Closing any ongoing stdin event
Jack-in terminal killProcess(): Killing process using tree-kill
Integration testing, Jack-in: waiting for connect...
Socket closed false
[2650:0602/163134.415966:ERROR:bus.cc(407)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")
Integration testing, Jack-in: waiting for connect...
[2650:0602/163147.058981:ERROR:bus.cc(407)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")
Integration testing, Jack-in: connected to repl
Integration testing, Jack-in: opened test.clj document again
Integration testing, Jack-in: test.lpy closed for Basilisp
    ✔ start repl and connect (jack-in) to Basilisp (14362ms)

@PEZ
Copy link
Collaborator

PEZ commented Jun 2, 2024

I don't quite understand why the CI pipeline is not running on this PR. Will try see if I can figure that out...

@ikappaki
Copy link
Contributor Author

ikappaki commented Jun 2, 2024

I don't quite understand why the CI pipeline is not running on this PR. Will try see if I can figure that out...

Indeed, I can't see anything suspicious with this PR. My fork's run for the same is at https://app.circleci.com/pipelines/github/ikappaki/calva/21/workflows/297663e1-16d7-4e29-a333-854a90048f9f

@PEZ
Copy link
Collaborator

PEZ commented Jun 2, 2024

Hmmm, I don't get it. Will merge it to dev since it looks good and you obviously see it running in CI, and passing, @ikappaki. Let's see if merging it to dev triggers the pipeline in the BetterThanTomorrow org.

@PEZ PEZ merged commit 2eea1c4 into BetterThanTomorrow:dev Jun 2, 2024
4 checks passed
@PEZ
Copy link
Collaborator

PEZ commented Jun 2, 2024

Yeah, https://app.circleci.com/pipelines/github/BetterThanTomorrow/calva/7938/workflows/84ced8f0-548b-4542-aa6d-053a0947e721

Very strange. I checked the settings on CircleCI and they still say to run on PRs from forks...

@ikappaki
Copy link
Contributor Author

ikappaki commented Jun 2, 2024

Yeah, https://app.circleci.com/pipelines/github/BetterThanTomorrow/calva/7938/workflows/84ced8f0-548b-4542-aa6d-053a0947e721

Very strange. I checked the settings on CircleCI and they still say to run on PRs from forks...

Perhaps worth raising a question with CI about it?

Thanks for merging!

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.

None yet

2 participants