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

[WIP] XDG support #3120

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Yamakaky
Copy link
Contributor

Yamakaky commented Jul 31, 2015

This PR is meant to bring XDG support to neovim. Features :

  • Expand XDG_* variables
  • Change default files location to match the XDG convention.

The [tmp] commit will be squashed before merge.

return xstrdup("/tmp/"); // Arbitrary value, any idea?
}

// We need to apprend the prefix to the HOME path.

This comment has been minimized.

@fmoralesc

fmoralesc Jul 31, 2015

Contributor

apprend -> append

This comment has been minimized.

@Yamakaky

Yamakaky Jul 31, 2015

Contributor

oops ^^

} else if (strcmp(name, "XDG_CACHE_HOME") == 0) {
home_suffix = "/.cache/";
} else if (strcmp(name, "XDG_RUNTIME_DIR") == 0) {
return xstrdup("/tmp/"); // Arbitrary value, any idea?

This comment has been minimized.

@ZyX-I

ZyX-I Jul 31, 2015

Contributor

I am using ~/.run. Note that you will have to create directory if it does not exist, and it has specific permission requirements (i.e. 0700: u=rw, group and everybody else have no permissions).

This comment has been minimized.

@ZyX-I

ZyX-I Jul 31, 2015

Contributor

Another option is ~/.local/run.

This comment has been minimized.

@fmoralesc

fmoralesc Jul 31, 2015

Contributor

Shouldn't the /run/... directory be used for stuff like this? I'm not sure how standard that is, though.

This comment has been minimized.

@ZyX-I

ZyX-I Jul 31, 2015

Contributor

@fmoralesc /run is pretty standard. But I do not know any standard option for user-specific ... (AFAIR there are some distribution-specific, but I do not remember where I saw this and do not find anything like this in my /run), so am using ~/….

This comment has been minimized.

@fmoralesc

fmoralesc Jul 31, 2015

Contributor

No, I meant a directory inside /run, not something called "..." inside it. Sorry for the confusion.

This comment has been minimized.

@jck

jck Jul 31, 2015

Contributor

In any case, it looks like nvim doesn't use XDG_RUNTIME_DIR at all?

This comment has been minimized.

@ZyX-I

ZyX-I Jul 31, 2015

Contributor

I think that $XDG_RUNTIME_DIR/nvim… is a proper default place for NeoVim sockets, much better then /tmp/nvimXXX/N.

This comment has been minimized.

@ZyX-I

ZyX-I Jul 31, 2015

Contributor

(Do not forget to call os_mkdir[_recurse]: with such default as ~/.local/run it is needed, and most likely exactly with _recurse because sometimes only ~ is present, but not ~/.local.)

This comment has been minimized.

@Yamakaky

Yamakaky Aug 1, 2015

Contributor

nvim doesn't use XDG_RUNTIME_DIR, but it should. I don't know what to modify, though. I think /tmp is a good default. See http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html#variables : it MUST NOT survive a reboot.

This comment has been minimized.

@ZyX-I

ZyX-I Aug 1, 2015

Contributor

@Yamakaky You have completely wrong priorities. /tmp is not 0700 and is not owned by the current user and that should have more priority because you are giving access to users that are not supposed to have it.

Note: XDG_RUNTIME_DIR according to the spec is not something that should be used like $XDG_RUNTIME_DIR/$USER/…, it is $XDG_RUNTIME_DIR/….

// We need to apprend the prefix to the HOME path.
char *value = vim_getenv("HOME");
value = xrealloc(value, strlen(value) + strlen(home_suffix) + 1);
strcat(value, home_suffix);

This comment has been minimized.

@fmoralesc

fmoralesc Jul 31, 2015

Contributor

clint is warning about this strcat()

This comment has been minimized.

@Yamakaky

Yamakaky Jul 31, 2015

Contributor

Saw that, on it.

This comment has been minimized.

@ZyX-I

ZyX-I Jul 31, 2015

Contributor

If you already have to compute length better use

const size_t value_len = strlen(value);
const size_t home_suffix_len = strlen(home_suffix);
value = xrealloc(value, value_len + home_suffix_len + 1);
memmove(value + value_len, home_suffix, home_suffix_len + 1);

This comment has been minimized.

@Yamakaky

Yamakaky Jul 31, 2015

Contributor

Done : 86e7c30

@@ -427,6 +427,31 @@ char *vim_getenv(const char *name)
return xstrdup(kos_env_path);
}

// XDG Base Directory expansion.
if (strncmp(name, "XDG_", 4) == 0) {

This comment has been minimized.

@justinmk

justinmk Jul 31, 2015

Member

Sneaking this into vim_getenv and making it do something totally different than what the caller asked, is unsettling.

This comment has been minimized.

@Yamakaky

Yamakaky Jul 31, 2015

Contributor

As it already expand $HOME and others, I thought it was appropriate, but I don't mind changing. I should have updated the doc, though.

This comment has been minimized.

@justinmk

justinmk Jul 31, 2015

Member

Let's see what others think. I haven't had chance to carefully think about it.

@marvim marvim added the WIP label Jul 31, 2015

Yamakaky added some commits Jul 31, 2015

@Yamakaky

This comment has been minimized.

Copy link
Contributor

Yamakaky commented Jul 31, 2015

The [tmp] commit will be squashed before merge.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Jul 31, 2015

I think nvim should still fallback on ~/.nvim/nvimrc and then ~/.nvimrc. How does this handle that?

@Earnestly

This comment has been minimized.

Copy link

Earnestly commented Jul 31, 2015

Let's just get everyone up to speed on what XDG_RUNTIME_DIR is and what the requirements on it are:

  • Used for non-essential, user-specific data files such as sockets, named pipes, etc.
  • Defaults to nothing, warnings should be issued if not set or equivalents provided.
  • Must be owned by the user.
  • Access mode must be 0700.
  • Filesystem fully featured by standards of OS.
  • Must be on the local filesystem.
  • May be subject to periodic cleanup.
  • Modified every 6 hours or set sticky bit if persistence is desired.
  • Can only exist for the duration of the user's login.
  • Shouldn't store large files as it may be mounted as a tmpfs.

Generally you don't create this directory, something like logind would. However if you do not use logind, you have to create it with the requirements listed above.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Jul 31, 2015

@fmoralesc Why do you think so? NeoVim is still not stable so it does not need to have backward compatibility with itself. Neither it needs to be backward compatible with Vim on this matter (it already is not). And making such changes as early as possible creates a good opportunity to throw away some legacy.

I think that making such fallbacks is adding more code for no real reason. It is not much work to move configuration files to the new location.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Jul 31, 2015

@Earnestly From NeoVim POV only those requirements are essential:

  1. 0700, owned by the current user, not shared, allows creating files and directories without any group and other access.
  2. Inside FS sockets and directories can be created and created with 0n00 on their own.

First are security requirements, second are functional requirements. Other requirements you listed is XDG specification requirements or common implementation details. When seeking directory to use you can test any requirement from the list, except for the shared status of the directory.

@jck

This comment has been minimized.

Copy link
Contributor

jck commented Jul 31, 2015

I think the following should be added to this PR:

  1. OSX and windows fallbacks need to be implemented for all XDG vars
  2. https://github.com/neovim/neovim/blob/master/src/nvim/msgpack_rpc/server.c#L38 should be modified to create sockets in $XDG_RUNTIME_DIR
@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Jul 31, 2015

Why do you think so?

@ZyX-I Because otherwise this will break every existing setup on merge. I think the better approach is to add this as an alternative first and remove the fallback later once the change is properly communicated and the changes are tested in the wild. And no, I don't think people read Following HEAD in the wiki (I don't think people will read anything, at all).

NeoVim ... does not need to have backward compatibility with itself.

I disagree, at least in this case.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jul 31, 2015

I think that making such fallbacks is adding more code for no real reason. It is not much work to move configuration files to the new location.

Yes, we agreed to that in #78. Especially if we get this in before 0.1.

I don't think people will read anything, at all).

I agree, but we need to play the alpha card here. Unless the fallback code is trivial, I don't want to spend a lot of time on it. We have very few users, really. We have more important things to work on than back-compat for a pre-alpha software. If we get XDG right, I think most will be more than happy with that.

Rust likely had more users than we do, and they broke things every month.

https://github.com/neovim/neovim/blob/master/src/nvim/msgpack_rpc/server.c#L38 should be modified to create sockets in $XDG_RUNTIME_DIR

👍

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jul 31, 2015

By they way, XDG support is going to be another strong out-of-the-box improvement, because we can enable backup/undo without dropping files everywhere.

@Yamakaky

This comment has been minimized.

Copy link
Contributor

Yamakaky commented Jul 31, 2015

@jck I'm not on windows so this PR is for unix only. See win_defs.h

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Jul 31, 2015

If we get XDG right, I think most will be more than happy with that.

Sure, I just worry some plugins might insta-break with this, and well, I'm a softie.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jul 31, 2015

Sure, I just worry some plugins might insta-break with this,

If it turns into a plugin disaster, we can revert and revisit. Ok?

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Jul 31, 2015

In those cases it will probably be the plugin's fault, really, most likely for hardcodding paths. Anyway, I don't think it's terribly likely to have many issues, but plugins that might need to modify the runtimepath (like plugin managers) have a higher chance of failing. If someone could test this, it would be great (I might do it later if none else cares).

@Yamakaky

This comment has been minimized.

Copy link
Contributor

Yamakaky commented Jul 31, 2015

BTW, I don't see why the travis build fails, the logs looks clean. What's wrong ?

const size_t home_suffix_len = strlen(home_suffix);
value = xrealloc(value, value_len + home_suffix_len + 1);
memmove(value + value_len, home_suffix, home_suffix_len + 1);
return value;

This comment has been minimized.

@ZyX-I

ZyX-I Aug 1, 2015

Contributor

Memory leak: vim_getenv returns an allocated value. home is neither used nor freed.

This comment has been minimized.

@Yamakaky

This comment has been minimized.

@justinmk

justinmk Aug 2, 2015

Member

Why not use concat_str() or vim_strcat() from nvim/strings.h?

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Aug 1, 2015

Latest variant with populating &runtimepath from various XDG_ variables is 280c536.

@fmoralesc Pathogen is not a plugin manager.

} else if (strcmp(name, "XDG_CONFIG_DIRS") == 0) {
return xstrdup("/etc/xdg");
} else if (strcmp(name, "XDG_DATA_DIRS") == 0) {
return xstrdup("/usr/local/share/:/usr/share/");

This comment has been minimized.

@ZyX-I

ZyX-I Aug 1, 2015

Contributor

Anybody knows why here are trailing slashes and above are not? (I have copied defaults from the specification.)

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Aug 1, 2015

pathogen is not a plugin manager

@ZyX-I I know, and I knew someone would comment on it. But it falls within the category I wanted to test, and it was simpler to describe it like this.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Aug 1, 2015

About plugins not fine with ~/.config/nvim: I have saw some and they are neither fine with

  1. ~/.nvim.
  2. Vim installed on Windows.
  3. According to the documentation: Vim installed on Amiga, OS/2, Macintosh, RISC-OS and VMS (literally everything, but linux).

: because they hardcode ~/.vim.

All plugin managers listed as not fine with the change allow configuring directories, correctly written plugins should either iterate over &runtimepath or use expand('<sfile>:…'). Note: VAM by default first checks whether it can install plugins to the location where it is installed on its own (and assumes that it is installed in pathogen style), so if user is not using system-wide VAM and also did not configure it to use some specific directory it is completely OK to just do mv --no-target-directory ~/.nvim ~/.config/nvim.

@justinmk justinmk referenced this pull request Aug 2, 2015

Closed

Newsletter vNext #110

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Aug 2, 2015

Commit bf4a342 adds escaping commas.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Aug 2, 2015

I am wondering whether anybody is actually supporting using XDG_*_DIRS for default &runtimepath.

@ZyX-I ZyX-I referenced this pull request Aug 2, 2015

Closed

[RFC] Add plugin management primitives #3125

3 of 3 tasks complete
#else
# define DFLT_RUNTIMEPATH \
"~/.nvim,$VIM/vimfiles,$VIMRUNTIME,$VIM/vimfiles/after,~/.nvim/after"
NVIM_CONF_DIR ",$VIM/vimfiles,$VIMRUNTIME,$VIM/vimfiles/after,"\
NVIM_CONF_DIR "/after"
#endif

This comment has been minimized.

@justinmk

justinmk Aug 2, 2015

Member

This entire #else block can be deleted, as well as all mentions of RUNTIME_GLOBAL in the source.

@@ -28,46 +32,51 @@
#ifndef SYNTAX_FNAME
# define SYNTAX_FNAME "$VIMRUNTIME/syntax/%s.vim"
#endif
// Not used anymore ?

This comment has been minimized.

@justinmk

justinmk Aug 2, 2015

Member

USR_EXRC_FILE, USR_VIMRC_FILE2, EXRC_FILE, can be removed. But not VIMRC_FILE.

This comment has been minimized.

@Yamakaky

Yamakaky Aug 2, 2015

Contributor

WIP. Do we also drop EXINIT ?

This comment has been minimized.

@justinmk
#ifndef EXRC_FILE
# define EXRC_FILE ".exrc"
#endif
// Not used anymore ?
#ifndef VIMRC_FILE
# define VIMRC_FILE ".nvimrc"

This comment has been minimized.

@justinmk

justinmk Aug 2, 2015

Member

This is for 'exrc' feature. Removing this is out of scope, will be a long discussion.

This comment has been minimized.

@Yamakaky

Yamakaky Aug 2, 2015

Contributor

... So what about EXRC_FILE you told me to drop ?

This comment has been minimized.

@justinmk

justinmk Aug 2, 2015

Member

Not related.

This comment has been minimized.

@Yamakaky

Yamakaky Aug 2, 2015

Contributor

Oh, sorry, I thought 'exrc' was related to .exrc

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 2, 2015

@ZyX-I

I am wondering whether anybody is actually supporting using XDG_*_DIRS for default &runtimepath.

I'm a little confused by your phrasing: are you saying that people wouldn't use this? Up until recently I wasn't aware of a use case for XDG_*_DIRS, but now...

I set XDG_DATA_DIRS=/usr/local/share:/home/michael/.racket/6.2/share so i3-dmenu-desktop can pick up /home/michael/.racket/6.2/share/applications/drracket.desktop. I do this because XDG_DATA_HOME is already set to ~/.local/share, and IIRC it can only contain a single directory path.

Admittedly, I can't think of a practical use case specific to nvim, since instead of dealing with environment variables, a user could just append to &rtp directly. Consider it food for thought I guess.

@justinmk justinmk added the defaults label Aug 2, 2015

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 2, 2015

As mentioned in #3125 (comment) , after the mechanics are decided here, this PR will require:

  1. Documentation. All references to ~/.vim and ~/.nvim should be found and corrected.
  2. Functional tests that exercise the default values and test that user overrides are respected.

We can probably move forward without deciding Windows defaults at the moment, but we'll need to address that shortly thereafter.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Aug 2, 2015

I'm a little confused by your phrasing: are you saying that people wouldn't use this? Up until recently I wasn't aware of a use case for XDG_*_DIRS, but now...

I am just asking whether this decision is supported by the developers.

Admittedly, I can't think of a practical use case specific to nvim, since instead of dealing with environment variables, a user could just append to &rtp directly. Consider it food for thought I guess.

This change is here to support XDG base directory specification. XDG_CONFIG_DIRS is there because specification requires to search configuration there. XDG_DATA_HOME+XDG_DATA_DIRS are there because /usr/share is a data directory and /usr/share/nvim is being used for plugin files (so we are already using directory from XDG_DATA_DIRS for our purposes).

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Aug 2, 2015

I set XDG_DATA_DIRS=/usr/local/share:/home/michael/.racket/6.2/share so i3-dmenu-desktop can pick up /home/michael/.racket/6.2/share/applications/drracket.desktop. I do this because XDG_DATA_HOME is already set to ~/.local/share, and IIRC it can only contain a single directory path.

It is rather strange setting:

  1. /usr/share is missing. It is from defaults and this will break whatever software installed by your system package manager which uses XDG_DATA_DIRS in place of hardcoding prefix=/usr somewhere. If there is such software.
  2. ~/… directory is after /usr/local. I would expect any user-local directory have greater priority.
@Yamakaky

This comment has been minimized.

Copy link
Contributor

Yamakaky commented Aug 2, 2015

BTW, do we want to support XDG on windows (with other defaults) ? I think it may simplify the code if we do that.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 2, 2015

Yes.

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 2, 2015

/usr/share is missing. It is from defaults and this will break whatever software installed by your system package manager which uses XDG_DATA_DIRS in place of hardcoding prefix=/usr somewhere. If there is such software

Indeed, only if there is such software. I'm using OpenBSD, so all 3rd-party software is in /usr/local, and there is no /usr/share/applications, so it works fine for me.

~/… directory is after /usr/local. I would expect any user-local directory have greater priority

Perhaps that's a good idea, although it hasn't been an issue in practice.

@jck

This comment has been minimized.

Copy link
Contributor

jck commented Aug 4, 2015

Rather than relying on macros, I think we should do something like this: jck@4131cff

That way, we can easily extend it to other win/osx and take care of creating directories if needed.

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 19, 2015

closing in favor of #3198

@ghost ghost closed this Aug 19, 2015

@jszakmeister jszakmeister removed the WIP label Aug 19, 2015

@Yamakaky Yamakaky deleted the Yamakaky:xdg-support branch Feb 21, 2016

This issue was closed.

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