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

Update PATH if the exact kitty executable path is not already present #1352

Closed
wants to merge 1 commit into from

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Feb 1, 2019

When I tried to test the new notification feature, I got ModuleNotFoundError: No module named 'kitty.update_check'. This is because I started the newer kitty version from inside an older kitty version. When kitty starts, it looks in PATH if there is a kitty executable already present. If so, it does not add anything new to PATH. But sinceupdate_check.py calls

p = subprocess.Popen([
    'kitty', '+runpy', 'from kitty.update_check import *; import time; time.sleep(2); print(get_released_version())'
], stdout=subprocess.PIPE)

and kitty was still the old kitty version, I got the error because the old version didn't have that module yet.
To reproduce (at least on macOS, didn't try on Linux):

  • git checkout 10f5461c (just some commit before this module became available)
  • make app
  • Move kitty.app somewhere else, e.g. the Desktop
  • Open kitty.app on the Desktop
  • cd to the kitty git directory
  • git checkout master
  • make app
  • kitty.app/Contents/MacOS/kitty
  • The error message of the second kitty instance will be printed in the first kitty instance

This pull request removes the functionality to check if a kitty exe is already in the path. main.py has a check if rpath and rpath not in items that only adds rpath to PATH if it is not already present. Do you see any issues with this fix? This is still not fool proof since the correct kitty version could be later in the path than a different, wrong version but it seems very unlikely that this would happen by accident.

@kovidgoyal
Copy link
Owner

This should be unneccessary, the problem should be fixed by: 13d478f

@kovidgoyal kovidgoyal closed this Feb 1, 2019
@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 1, 2019

My steps to reproduce still work, I can still get the ModuleNotFoundError. That's because kitty_exe() tries to find the kitty binary in the PATH and returns that if it finds something. Because of that, the exact same kitty binary is executed with or without your patch. But I'm not very concerned about the ModuleNotFoundError, I'm more concerned about the PATH variable not having the correct path to the current kitty binary.
To see what I mean (steps for macOS but Linux is probably similar):

  • Have two kitty.app in different locations
  • Launch one of them
  • echo $PATH shows that the correct kitty binary location is inside PATH
  • cd to the directory that contains the other kitty.app
  • Launch the second instance with kitty.app/Contents/MacOS/kitty
  • echo $PATH shows only the kitty binary location from before, not the new one
  • This can be a problem if the two kitty.app are different versions

My commit should fix this. What was the reason that you implemented it this way? Maybe I'm misunderstanding something.

@kovidgoyal
Copy link
Owner

No, kitty_exe() looks for bundle_exe_dir first. Which means on macOS it will always return the path to the correct exe, so I dont see how your steps could work, unless you are running an unbundled kitty, from source. In which case it is up to you to setup PATH correctly.

@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 2, 2019

I modified the code to print rpath right after rpath = getattr(sys, 'bundle_exe_dir', None) but it was None both when when executing the kitty.app/Contents/MacOS/kitty binary and the kitty.app/Contents/Frameworks/kitty/kitty/launcher/kitty script. I didn't run an unbundled kitty. In my steps to reproduce I opened the first kitty instance via double click on kitty.app and the second instance by starting kitty.app/Contents/MacOS/kitty from the shell inside the first kitty instance.

On a side-note I'm wondering what the difference between kitty.app/Contents/MacOS/kitty and kitty.app/Contents/Frameworks/kitty/kitty/launcher/kitty is. The first one seems to be the compiled version of linux-launcher.c and the other one the kitty/launcher/kitty script.

@kovidgoyal
Copy link
Owner

kitty.app is not bundle based (it uses system python to run kitty), so that wont work for it, in this case you will be fine unless you either have a wrong kitty in PATH or you happen to launch kitty from inside kitty and there are incompatible changes, which is rare. launcher relies on system python to run kitty, linux-launcher works with the app bundle.

In any case, I have fixed it not to care if executing the update check fails.

@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 2, 2019

Ok, so if I compile kitty from source with make app, it's not bundle based because it's using the system python but if I use the precompiled version, it is bundle based because it has its own python. Is that correct? If so, sorry for the little misunderstanding.
You're right, my steps to reproduce don't work with the precompiled version. I don't understand why though. I'd still like the new kitty binary to be added to the PATH. What is the code I removed for? Maybe it can be fixed another way.

@kovidgoyal
Copy link
Owner

Yes, that is correct. Using the launcher script as the kitty binary
assumes there exists a python3 executable on the system PATH somewhere,
which may or may not be true. Which is why kitty_exe() will first look for
an existing kitty executable on PATH, since it cannot assume there is a
python3 on PATH and even if it does, it cannot know if that python3 is
the one against which the user built kitty.

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.

2 participants