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

Resolve the executable path with the configured env PATH #4324

Closed
page-down opened this issue Dec 7, 2021 · 12 comments
Closed

Resolve the executable path with the configured env PATH #4324

page-down opened this issue Dec 7, 2021 · 12 comments

Comments

@page-down
Copy link
Contributor

page-down commented Dec 7, 2021

Is your feature request related to a problem? Please describe.
The configured env PATH is not used for executable path resolution when using shortcut launch or remote control launch command.
This results in not loading the expected program.

Describe the solution you'd like
Like the process of loading the shell at startup, the location of the executable can be resolved from the env PATH.

Describe alternatives you've considered
n/a

Additional context
macOS does not have /usr/local/bin in the default PATH.

  • legacy bash: /bin/bash
  • latest bash: /usr/local/bin/bash
  • bash 5.1 <- PATH=/bin /path/to/kitty --config=NONE -o env=PATH=/usr/local/bin:/bin -o shell=bash
  • bash 3.2 <- -o 'map f1 launch bash'

When the legacy version of bash is in /bin and the latest version of bash is in /usr/local/bin, the latest version of bash will be loaded at startup when env PATH is configured correctly, and the legacy version of bash will be loaded when launch bash is executed.

For example:

PATH=/bin kitty/launcher/kitty --config=NONE -o env=PATH=/usr/local/bin:/bin -o 'shell bash --noprofile --norc' -o allow_remote_control=yes sh -c 'kitty @ launch; kitty @ launch bash --noprofile --norc'

After executing the above command, bash 5.1 will be launched, and pressing ctrl+shift+t to create a new tab will also be bash 5.1. However, executing kitty @ launch bash will run bash 3.2.

Other features that use kitty/utils.py find_exe() may also be affected. Perhaps a generic infrastructure for executing (finding) commands in a child environment is needed, just like starting the shell at boot time, without passing env PATH every time.

@kovidgoyal
Copy link
Owner

I dont see why this is a bug. env sets the environment in which
child processes launched by kitty operate. It does not set the environment in
which kitty itself operates. launch runs a child that is located in the
environment kitty operates.

@page-down
Copy link
Contributor Author

No, it was never a bug, so I marked it as enhancement. The reason is obvious as you said.

There is currently a situation where it is not convenient to set kitty's own environment variables. Based on that I suggest to be able to use the user-configured PATH when looking for executables as well.

Currently, when you launch kitty, the shell already resolves from the env PATH. It might be possible to improve the kitty experience by making it easier to launch as well.

It is because of the infrastructure that already exists and is very convenient which is why I am suggesting it. Otherwise, under macOS, the user needs to use some hacky tricks or specify absolute paths.

Maybe you have a better approach, thanks for your understanding.

@kovidgoyal
Copy link
Owner

env is certainly not the right way to do it, since it is meant for child
environments. And I dont really understand the issue here, why is it
difficult to adjust the PATH on macOS? Do launchd.conf and/or launchctl no
longer work? I am not keen to add a means for kitty to modify its own
PATH just to work around Apple's poor design.

@page-down
Copy link
Contributor Author

Yes, in principle I agree with you.

Just some of my personal thoughts, whether it could improve ease of use for users while being logically correct, without bringing maintenance difficulties.

If you don't think the benefits of introducing these improvements are worth it, then I won't mention it again.

So should PATH=/bin /path/to/kitty --config=NONE -o env=PATH=/usr/local/bin:/bin -o shell=bash be loaded with /bin/bash?
The above command is currently running /usr/local/bin/bash.

I don't remember the behavior of previous versions, e.g. whether it would cause backward compatibility issues.
Or simply don't bother with it and leave it as is.


I recently saw something about Microsoft Windows backwards compatibility s***, where issues with clear logical errors are not fixed for the sake of backwards compatibility. A bug becomes a permanent feature.

@page-down
Copy link
Contributor Author

page-down commented Dec 8, 2021

I guess I was wrong, /usr/local/bin seems to be included in the macOS default configuration.

def find_exe(name: str) -> Optional[str]:
    import shutil
    ans = shutil.which(name)
    if ans is None:
        # In case PATH is messed up
        if is_macos:
            paths = system_paths_on_macos()

It's because bash is already found in the PATH passed in by launchd, so it's not looking in system_paths_on_macos().

If the user installed the latest version of bash in /opt/local/bin (perhaps the location of some package manager), it will not work even if it is added to macOS /etc/paths.d/.

EDIT: Sorry for wasting your time, I need to look into it more closely.

This is the current situation

  • macOS Launchd: /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
  • /etc/paths: Same as above
  • If you add /opt/local/bin/ to /etc/paths.d/, then you want to run the bash in it, it will not work. Because kitty PATH is preferred. This is by design, and this is the correct way to handle it.

All that remains is whether the shell's preferred PATH location needs to be fixed.

I did the following tests with kitty 0.23.1 release version

sudo ln -s /usr/bin/busybox /usr/local/bin/sh
PATH=/bin ./kitty/bin/kitty --config=NONE -o env=PATH=/usr/local/bin:/bin -o shell=sh
# -> busybox sh

PATH=/bin ./kitty/bin/kitty --config=NONE -o shell=sh
# -> dash

@kovidgoyal
Copy link
Owner

Yes, if you want to use a bash not in your PATH or that comes second in
your PATH the obvious thing to do is use an absolute path. Or modify
your PATH or create a symlink, its not to add functionality to every
program to override PATH for you.

@page-down
Copy link
Contributor Author

Yes, thank you for your patience.

I tested the shell's default PATH under Linux. Edited in the above comment. Do you think it needs to be fixed?

@kovidgoyal
Copy link
Owner

Note sure what you are asking, are you saying the env option is affecting resolution of the shell?

@page-down
Copy link
Contributor Author

Yes, when I specify env PATH, the shell executable will resolve from that path.

@page-down
Copy link
Contributor Author

page-down commented Dec 8, 2021

I noticed that in the latest commit kitty/utils.py
paths = os.pathsep.split(ep) should be paths = ep.split(os.pathsep) ?

Otherwise, kitty's PATH will not be used.


I spent some time figuring out how to configure GUI environment variables under macOS.

To set kitty's PATH to work only for launch action or shell option, the user would have to modify kitty.app/Contents/Info.plist LSEnvironment, and re-sign the app, or modify the source code.

If you don't want to make changes to kitty, or load python from the DEV folder, or modify os.environ in python:
You could only configure PATH globally, which is perhaps too wide and risky.

Takes effect globally, requires sudo root permission and no official command to restore:

sudo launchctl config <scope: system|user> path /opt/local/bin:/bin

The command launchctl setenv cannot be used to set the PATH environment variable and PATH will be reset to a fixed value by launchd.

It seems that under macOS, this issue can only be solved by the GUI program itself.

I can't believe this was done by a two trillion dollar company, and I can understand you do not want to provide options for this mess.
I can modify the source code myself, thanks for your continuous improvement for kitty.

@kovidgoyal
Copy link
Owner

I decided to go ahead an implement an option to control this after all.

@page-down
Copy link
Contributor Author

Thank you very much.

In macOS, launchd.conf is deprecated due to security issues, and the launchctl setenv API does not allow global changes to the PATH.

This option solves the problem.

Users of Apple Silicon Macintosh can set (prepend) the search path for those package managers that are installed under /opt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants