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

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

Merged
merged 4 commits into from
Jul 31, 2024

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
57 checks passed
@charliermarsh charliermarsh added the bug Something isn't working label Jul 31, 2024
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.

3 participants