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

Set default fallback path when XDG_DATA_DIRS is empty #4199

Closed
wants to merge 1 commit into from

Conversation

page-down
Copy link
Contributor

When XDG_DATA_DIRS is empty, setting the path will cause the default fallback path to not take effect.

So append the default fallback path when it is empty.

Fixes:

#3948 (comment)

@rien333
Copy link

rien333 commented Nov 6, 2021

Thanks for the quick fix ! I'll test this when I get back to my PC (though it probably works)

@rien333
Copy link

rien333 commented Nov 6, 2021

I'm worried that the two paths you're adding now don't exhaust the default locations (though then again, I haven't really tried to figure out what's going on in your code). From the xdg spec:

$XDG_DATA_HOME defines the base directory relative to which user-specific data files should be stored. If $XDG_DATA_HOME is either not set or empty, a default equal to $HOME/.local/share should be used.

Indeed, I don't have XDG_DATA_HOME set, but I still make use of $HOME/.local/share to store user specific .desktop files.

@page-down
Copy link
Contributor Author

We do not set XDG_DATA_HOME, so it is not affected. If the value is empty, it will remain empty afterwards as well.

Depending on your situation, you might want to get your environment variables configured properly. Otherwise you may also run into problems elsewhere. :)

It seems ridiculous to me to expect every GUI software to check and set the fallback path on its own before adding to XDG_DATA_DIRS.

@kovidgoyal
Copy link
Owner

why are we not setting it on macOS. While xdg-open doesnt exists there,
there is probably other software that uses it.

@page-down
Copy link
Contributor Author

why are we not setting it on macOS. While xdg-open doesnt exists there,
there is probably other software that uses it.

One problem with this solution is that the paths may be different on different Linux distributions, even on macOS.

For some unusual Linux distributions, the default paths that do not use this fallback path will not take effect.

For devices using Apple Silicon in combination with homebrew, it also does not work, as far as I know.

So is it necessary to check every possible path and then add them?

In fact, if there is a special need to use it, it is better to set it in config env by the user.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Nov 6, 2021 via email

@page-down
Copy link
Contributor Author

Are they actually different? And if they are where are they defined?

https://docs.brew.sh/Installation

/opt/homebrew/share

I am uncomfortable with adding a specific package manager path to kitty code, it means one more dependency and you have to track and maintain changes to this package manager.

That's fine if you want.

The problem is we cant have a default setup that breaks many users software
requiring them to change config.

Yes, that's for sure.

It would solve the problem in the most common cases if the path above could also be added.

@kovidgoyal
Copy link
Owner

Not sure I follow. Shouldn't brew be adding that value itself somewhere,
why do we need to add it? And what if the user has multiple brew
installs or installs in non-standard locations or uses macports or fink
or whatever.

@page-down
Copy link
Contributor Author

From my understanding, the situation is like this:

  • kitty should not cause trouble for users on supported platforms with default configurations.

  • kitty needs to add its own data dirs path to the XDG_DATA_DIRS environment variable to support related functions.

  • The default fallback path for XDG_DATA_DIRS is the path /usr/local/share:/usr/share defined by the specification in most cases on Linux systems.

  • macOS does not use any XDG specification by default. So software that uses this specification, as this comes from the user's chosen package manager, is entirely a personal thing for the user.

  • We have no way to know exactly where this fallback path is on each system and each package manager.

  • Software should generally have its own XDG_DATA_DIRS fallback path, which is maintained by the distribution packager, for example by providing a patch for the software to resolve compatibility.

  • For example, xdg-open has a default fallback path, when XDG_DATA_DIRS is empty, it falls back to the path maintained by the packager.


Not sure I follow. Shouldn't brew be adding that value itself somewhere,
why do we need to add it?

In general, yes, what the packager really maintains is: the default fallback path when XDG_DATA_DIRS is empty.

For distributions, all normal distributions, running X11/Wayland, comes with the default XDG_DATA_DIRS environment variable, which is also maintained by the packager.

The root of the problem is that kitty is not maintained by these packagers, and if it is maintained by them, then it is definitely working properly with other software.


My personal opinion is that everything that is not the default should be configured by the user.

Because macOS itself does not come with any default xdg path specification. We have no way of knowing which package managers are installed and which custom paths are used.

The problem mentioned today is also caused by the system not being properly configured with environment variables, as Linux systems are installed and configured by the user. Normally, this problem should be properly solved by the maintainer of the linux distribution.

We cannot support all cases that are beyond normal.

@kovidgoyal
Copy link
Owner

I dont think I like this approach. XDG_DATA_DIRS is too fragile. And
there is no robust way to set it. And because environment variables are
inherited by all child process it can potentially have vast
side-effects.

I suggest either of the following approaches:

  1. Remove the shell integration script path from XDG_DATA_DIRS in the kitty fish shell
    integration script on startup. This, as you pointed out, means no shell
    integration if you run fish inside fish, however, that is broken already
    since the KITTY_SHELL_INTEGRATION env var is deleted.

1.5) Delete XDG_DATA_DIRS and alias fish to a function that sets both
XDG_DATA_DIRS and KITTY_SHELL_INTEGRATION before running the real fish.
I dont know what, if anything aliasing fish inside fish will break.

  1. Go back to using symlinks in $XDG_CONFIG_HOME/fish

  2. Use symlinks in $XDG_DATA_HOME/fish

@page-down
Copy link
Contributor Author

I dont think I like this approach.

Yes, I didn't like it either. The final conclusion is that you can't leave an XDG_DATA_DIRS environment variable with only kitty data dir.

Is it appropriate to remove it when there is only one kitty data dir left? This does not affect the majority of users on Linux who already have the correct XDG_DATA_DIRS, they can continue to run fish in fish once they have configured KITTY_SHELL_INTEGRATION.

@kovidgoyal
Copy link
Owner

Well if you want to use the (1) approach above there are some subtleties to consider.

a) check that after removing XDG_DATA_DIRS completion for the kitty command is still loaded from the correct place

b) As for the actual removal, the simplest would be to set another env var called say KITTY_FISH_XDG that contains the value of the XDG_DATA_DIRS variable before kitty changes it. If there was no XDG_DATA_DIRS variable it should be unset. If it was empty it should be empty but set. And so on. Then the integration script can simply set/delete XDG_DATA_DIRS to that value and then delete KITTY_FISH_XDG

@page-down
Copy link
Contributor Author

It's really too bad. We can't have nice things because of this terrible environment.

This approach doesn't look like it's going to work either. Launching any program will cause XDG_DATA_DIRS to be used.

kitty -o 'map f1 launch sh -c "env|grep XDG;read"'

This is a dead end. Never thought that a small fallback path would make such a general purpose environment variable totally unusable.

@page-down
Copy link
Contributor Author

I'm working on the second approach.

Only users who have XDG_DATA_DIRS configured correctly will have the nice thing and won't need to clean up the XDG_DATA_DIRS environment variable.

Those that don't have it configured fall back to XDG_CONFIG_HOME symlink by default.

May I ask if this is acceptable to you?

BTW: The second approach has a problem with loading mismatched integration scripts when running multiple versions of kitty at the same time, especially for kitty development.

@kovidgoyal kovidgoyal closed this in 35514e0 Nov 7, 2021
@kovidgoyal
Copy link
Owner

No need, I have applied approach 1 without polluting the evironment for
launch

@page-down
Copy link
Contributor Author

page-down commented Nov 7, 2021

Thank you very much.

Also the environment variables need to be set with set --global --export.

set --export XDG_DATA_DIRS "$KITTY_FISH_XDG_DATA_DIRS"

@page-down
Copy link
Contributor Author

The current implementation has a problem where the custom XDG_DATA_DIRS configured by the user in ~/.config/fish/ is overwritten by the configuration from kitty after the integration script.

# ~/.config/fish/conf.d/user-xdg-conf.fish
# This should be the value that the user expects.
set -gx XDG_DATA_DIRS /tmp/from-user-conf-d:/usr/local/share:/usr/share
XDG_DATA_DIRS=/tmp/from-parent-env kitty --config=NONE -o shell=fish -o 'env XDG_DATA_DIRS=/tmp/from-kitty-conf-env'

# after user config
XDG_DATA_DIRS=/tmp/from-user-conf-d:/usr/local/share:/usr/share
KITTY_FISH_XDG_DATA_DIRS=/tmp/from-kitty-conf-env

# first prompt, after integration script
XDG_DATA_DIRS=/tmp/from-kitty-conf-env

Maybe you have a better solution for this.

@page-down page-down deleted the fix-xdg-data-dirs branch November 7, 2021 09:45
@kovidgoyal
Copy link
Owner

Should be fine now

@page-down
Copy link
Contributor Author

page-down commented Nov 7, 2021

Actually, I was trying to define the correct XDG_DATA_DIRS in fish conf.d to enable shell integration for the command fish --private in fish, but the defined shell-integration path was removed from it.

I want to enable shell integration for running fish --private in the following kitty instance.

kitty --config=NONE -o shell=fish
set -gx KITTY_SHELL_INTEGRATION enabled
set -gx --path XDG_DATA_DIRS /path/to/kitty/shell-integration /path/to/others/dirs

@kovidgoyal
Copy link
Owner

Alias fish to a function that sets the env vars and runs the real fish

@page-down
Copy link
Contributor Author

page-down commented Nov 7, 2021

Alias fish to a function that sets the env vars and runs the real fish

Unfortunately, this does not work. fish does not like being alias. unless renamed.

# with: alias fish="ENV=value fish"
- (line 1): The function 'fish' calls itself immediately, which would result in an infinite loop.

Is this feasible?

KITTY_CHILD_FINAL_XDG_DATA_DIRS=/path/to/all/xdg/dirs
KITTY_FISH_XDG_DATA_DIR=/path/to/shell-integration
XDG_DATA_DIRS=/user/conf.d/defined

if test "$XDG_DATA_DIRS" != "$KITTY_CHILD_FINAL_XDG_DATA_DIRS"
# keep the user defined XDG_DATA_DIRS
end

EDIT: It doesn't seem to work, the user can set it to the same path. The case is indistinguishable.

@kovidgoyal
Copy link
Owner

You dont call fish inside fish you use \fish (at least that works to
avoid aliases in POSIX shells)

@kovidgoyal
Copy link
Owner

@page-down
Copy link
Contributor Author

page-down commented Nov 7, 2021

Thanks for the reminder, almost forgot about this. That looks like it's working fine. I'm not so sure if there will be problems elsewhere.

I can't think of a better way. So I'll leave it for now.


EDIT:

Sorry for not being able to notice that the exit path is already there.

set -g KITTY_SHELL_INTEGRATION enabled
set -gx XDG_DATA_DIRS $KITTY_FISH_XDG_DATA_DIR:/path/to/system/dirs
set -e KITTY_FISH_XDG_DATA_DIR

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.

3 participants