Skip to content

fix(venv): make relocatable activation scripts support ksh#5640

Merged
charliermarsh merged 4 commits intoastral-sh:mainfrom
paveldikov:relocatable
Jul 31, 2024
Merged

fix(venv): make relocatable activation scripts support ksh#5640
charliermarsh merged 4 commits intoastral-sh:mainfrom
paveldikov:relocatable

Conversation

@paveldikov
Copy link
Contributor

It transpires that detecting the directory a script was sourced from is non-trivial across bash, ksh and zsh.

The previous version was a one-liner and supported bash and zsh but not ksh.

It is possible to keep the one-liner and add ksh support, but that is mutually-exclusive with zsh.

Therefore, the only way to square this circle is to add an if block. A silver lining here is that although longer, the script is probably easier to follow as there is less code-golfing going on.


© 2024 Morgan Stanley.

THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE APACHE LICENSE V.2.0 AND THE MIT LICENSE.

THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.

@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

Can you note why ksh is important to support in particular? There are a lot of shells out there. Are we going to support all of them?

@paveldikov
Copy link
Contributor Author

Can you note why ksh is important to support in particular? There are a lot of shells out there. Are we going to support all of them?

I agree that trying to support all of them is futile.

The Korn shell in particular is very much entrenched in a lot of enterprise environments at least.

Not sure about zsh -- I recall ohmyzsh being trendy some years ago... never used it much myself. I personally do not object to excluding it, but others might?

@charliermarsh
Copy link
Member

I think Zsh is very important, I believe it's the default on macOS.

@charliermarsh
Copy link
Member

I'm fine with this as it's only used for --relocatable, right?

@paveldikov
Copy link
Contributor Author

I'm fine with this as it's only used for --relocatable, right?

Yes. I will actually go back and drop the sourced'ness checks for all except bash (which is what currently exists). I am testing them right now and are really brittle stuff -- would not want them to impact real users.

It transpires that detecting the directory a script was sourced from
is non-trivial across `bash`, `ksh` and `zsh`.

The previous version was a one-liner and supported `bash` and `zsh` but
not `ksh`.

It is possible to keep the one-liner and add `ksh` support, but that
is mutually-exclusive with `zsh`.

Therefore, the only way to square this circle is to add an `if` block.
A silver lining here is that although longer, the script is probably
easier to follow as there is less code-golfing going on.
@paveldikov paveldikov marked this pull request as draft July 30, 2024 23:21
@paveldikov
Copy link
Contributor Author

Phew, that was not fun. Tested (by hand) across bash, ksh and zsh. So far so good. zsh in particular is where the > /dev/null trick comes from -- TIL it has a 'loud' cd command.

@paveldikov paveldikov marked this pull request as ready for review July 30, 2024 23:40
@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

I guess my qualm with ksh (and curiosity as to why it is important) is that it's not included in our existing uv-shell::Shell variants. I'd like to have a solid list of shells we support, rather than support for some here and others there.

I presume it's present in enterprise because of red hat?

@paveldikov
Copy link
Contributor Author

I guess my qualm with ksh (and curiosity as to why it is important) is that it's not included in our existing uv-shell::Shell variants. I'd like to have a solid list of shells we support, rather than support for some here and others there.

That is a fair concern. I had a brief look at the module and it doesn't look terribly complicated (save for configuration_files() which requires a bit of research because I haven't got a clue as to how it's used). How would you like me/us to approach this -- same PR or track separately?

I presume it's present in enterprise because of red hat?

The correlation is rather uncanny indeed. Although I would not be surprised if that the affinity for ksh predates widespread RHEL (or even Linux!) adoption.

@paveldikov
Copy link
Contributor Author

Ok @zanieb it turns out it was a fairly trivial addition considering there's already a pattern of supporting multiple POSIX shells with largely the same code.

@charliermarsh charliermarsh added the compatibility Compatibility with a specification or another tool label Jul 31, 2024
@charliermarsh
Copy link
Member

Thank you. This seems reasonable to me, especially now that we added Ksh to our shell enum.

@charliermarsh charliermarsh merged commit d05f2b2 into astral-sh:main Jul 31, 2024
@charliermarsh charliermarsh added the bug Something isn't working label Jul 31, 2024
charliermarsh added a commit to astral-sh/python-build-standalone that referenced this pull request Dec 3, 2024
## Summary

The current shebang seems to fail when the path itself contains spaces. For example:

```
❯ "/Users/crmarsh/Library/Application Support/uv/python/cpython-3.13.0-macos-aarch64-none/bin/pydoc3"
/Users/crmarsh/Library/Application Support/uv/python/cpython-3.13.0-macos-aarch64-none/bin/pydoc3: line 2: /Users/crmarsh/Library
Support/uv/python/cpython-3.13.0-macos-aarch64-none/bin/python3.13: No such file or directory
/Users/crmarsh/Library/Application Support/uv/python/cpython-3.13.0-macos-aarch64-none/bin/pydoc3: line 2: exec: /Users/crmarsh/Library
Support/uv/python/cpython-3.13.0-macos-aarch64-none/bin/python3.13: cannot execute: No such file or directory
```

This PR replaces it with the shebang that we use in uv for relocatable environments, which is outlined in the following series of PRs and issues:

- https://github.com/astral-sh/uv/pull/5515/files#r1694358328
- astral-sh/uv#5640
- astral-sh/uv#8079

Closes #394.

Closes astral-sh/uv#9348.
@jsirois
Copy link

jsirois commented Dec 13, 2024

Late to this, but, FWIW, ksh is the default shell on AIX which I think a fair number of IBM customers still use. So, not silicon-valley, but certainly banks and the like.

zanieb pushed a commit that referenced this pull request Sep 5, 2025
## Summary
This refreshes the venv activation scripts from upstream `virtualenv`
project.
This was largely triggered by a problem in the activate.nu script (for
nushell):
- #14888 
- #14914 
- #14917 

I was careful to respect the git history going back to #3376
(the last time this was done).
Actually I looked at the complete history from back when this
`uv-virtualenv` crate was named after a Pokémon (⁉️), but I found
nothing (about activation scripts) from back then that hasn't been
overwritten since.

### Some post-processing was involved

- Retained license info at top of scripts
- Retained template vars (eg `{{ BIN_PATH }}`) to assure current support
toward relocatable venv
- Retained deviation from upstream in #5640. This seems to
be the only deviation that isn't in sync with upstream.

### Notable changes from upstream

- (omitted due to undesirable complexity) pypa/virtualenv#2928 and its
follow-up pypa/virtualenv#2940
- pypa/virtualenv#2910 (what prompted #14917 from
#14888)

## Test Plan

There was a request in #14917 to add unit tests to detect breakage or
errors.
I have added a CI job that runs the nushell activation script.
But I think it is better to have the CI test all/most supported shells.
See also #15294 

I have tested this locally using

- [x] nushell (v0.106.1)
- [x] cmd.exe (Microsoft Windows [Version 10.0.26100.4946])
- [x] bash in WSL (GNU bash, version 5.1.16(1)-release
(x86_64-pc-linux-gnu))
- [x] pwsh (PSVersion 5.1.26100.4768)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working compatibility Compatibility with a specification or another tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants