basilisp-lang / basilisp

A Clojure-compatible(-ish) Lisp dialect targeting Python 3.9+
https://basilisp.readthedocs.io
Eclipse Public License 1.0
276 stars 8 forks source link

`basilisp` executable limited local namespace access #1027

Closed ikappaki closed 1 month ago

ikappaki commented 2 months ago

The basilisp executable does not include any local paths in the PYTHONPATH by default, aside from those inherited from the environment. This can limit a developer's ability to access local namespaces, often leading to non-portable workarounds across different projects or platforms.

For instance, if you invoke the basilisp executable from outside a project, it won't automatically search the current working directory to resolve namespaces. As a result, you wouldn't be able to require any local files unless additional configuration steps are made.

A workaround is to manually set the PYTHONPATH inline with the basilisp command, as suggested in issue #952. However, this approach has its limitations, particularly on Windows. On that platform, environment variables can only be set globally within the shell, not on a per-command basis like this:

PYTHONPATH=src basilisp run -n a

Additionally in a project, the method for configuring PYTHONPATH through Python build tools can vary. For example, in a typical Basilisp project, code is located in the src/<project> directory and tests are in the tests/<project> directory. When using Poetry, the package configuration in pyproject.toml specifies where the code to be packaged is located, usually src/<myproject>:


[tool.poetry]
name = "project"
version = "0.1.0"
description = ""
authors = []
readme = "README.md"
packages = [
    { include = "project", from = "src" },
] 

This setup adds /path/to/<myproject>/src to the PYTHONPATH after running poetry install, via an entry created in <venv>/<python>/site-packages/<project>.pth. Consequently, while the basilisp executable can access namespaces in src/, it won't have access to those in the tests/ directory since the current working directory ./ isn't included in the PYTHONPATH.

I couldn't find a straightforward way to add an extra local directory to PYTHONPATH using Poetry outside of the packages option. IMHO, developers shouldn't have to spend time figuring out hacks for this with any Python build tool either.

One potential solution for both standalone and project scenarios is for the basilisp executable to automatically add the current working directory to the PYTHONPATH. This behavior is consistent with the python interpreter, which, according to the Python sys.path documentation, prepends an empty string to sys.path (representing the current working directory) when running code from the command line or REPL.

However, the basilisp executable currently does not include the empty string in the PYTHONPATH. Instead, it prepends the directory where the basilisp executable resides (e.g., /home/user/.cache/pypoetry/virtualenvs/myproject-8F5vs53K-py3.10/bin).

To address this issue, one solution could be to have the basilisp executable prepend the empty string '' to the PYTHONPATH on startup, while still inheriting other environment variables as usual. Another, more flexible solution familiar to Clojure developers would involve specifying local paths in the Clojure-like configuration file, in our case, basilisp.edn, as proposed in issue #900.

What do you think? The second solution offers greater flexibility and familiarity to Clojure developers, while the first is simpler to implement but includes the entire current working directory in the PYTHONPATH.

Thanks

Note: When the basilisp script is invoked from within the Basilisp project, it does include the empty string '' in the PYTHONPATH. This occurs because poetry install sets up basilisp as a shell script in the virtual environment, which ultimately calls the python executable. As a result, the empty string '' is prepended to the PYTHONPATH by the Python interpreter. However, when Basilisp is installed as a package, the basilisp script is a binary executable, which does not call python and, therefore, does not prepend '' to the PYTHONPATH but the path where the executable resides.

chrisrink10 commented 2 months ago

I think in the short term it would be fine to prepend an empty string to sys.path from the CLI, like Python does.

I don't think I want the actual tool basilisp concerning itself with basilisp.edn. I have some plans to eventually create some sort of project management tool that would be responsible for reading e.g. basilisp.edn and applying the settings you're talking about.

chrisrink10 commented 2 months ago

See #1036 for what I'm thinking for now

ikappaki commented 1 month ago

I think in the short term it would be fine to prepend an empty string to sys.path from the CLI, like Python does.

thanks for looking into this!

See #1036 for what I'm thinking for now

Adding a CLI option to extend PYTHONPATH is valuable and good to have, but it might not fully address a key issue with the nREPL server. For instance, when an editor starts it with basilisp nrepl-server, it still won't recognize the ./tests directory if the Python build tool is configured to look in ./src. While the editor's command to invoke the Basilisp nREPL server can usually be changed, it requires users to have a solid understanding of how PYTHONPATH is setup in Basilisp and their editor. They would need to identify why certain namespaces outside ./src aren't loading and figure out how to adjust the editor configuration to include them.

This was my main reason for advocating basilisp.edn to manage paths. As you mentioned, this will be handled by the Basilisp build tool, which I'm looking forward to.

Do you see any issue with also prepending the empty directory to sys.path by default when basilisp starts, to at least cover the ./tests case as initially suggested?

chrisrink10 commented 1 month ago

Do you see any issue with also prepending the empty directory to sys.path by default when basilisp starts, to at least cover the ./tests case as initially suggested?

Indeed this is buried in the linked PR -- sorry if that wasn't clear.

ikappaki commented 1 month ago

Do you see any issue with also prepending the empty directory to sys.path by default when basilisp starts, to at least cover the ./tests case as initially suggested?

Indeed this is buried in the linked PR -- sorry if that wasn't clear.

Oh, sorry, I missed that 😅. I've added a minor comments to the patch with regards to the name of the operation. Thanks