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.

Sign up for GitHub

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

Jump to bottom

[WIP] XDG support #3120

Closed
wants to merge 15 commits into from
Closed

[WIP] XDG support #3120

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9c50903
Add defaults for the expansion of XDG_* env variables.
Yamakaky May 2, 2015
00ff3da
Add a convenient define for the config files paths
Yamakaky Jul 8, 2015
318b69a
Use init.vim as the default config file name
Yamakaky Jul 8, 2015
793bb3c
Move viminfo to NVIM_DATA_DIR
Yamakaky Jul 8, 2015
a142616
Move the swap files to NVIM_DATA_DIR
Yamakaky Jul 8, 2015
d4fb8fe
Move the 'directory' to NVIM_DATA_DIR
Yamakaky Jul 8, 2015
14cb32b
Move 'viewdir' to NVIM_DATA_DIR
Yamakaky Jul 8, 2015
faa11d5
Add NVIM_CONF_DIR to RUNTIMEPATH
Yamakaky Jul 8, 2015
86e7c30
[tmp] fix a warning for use of strcat
Yamakaky Jul 31, 2015
beebfb9
[tmp] fix line length
Yamakaky Jul 31, 2015
81302c6
[tmp] use NVIM_*_DIR
Yamakaky Aug 1, 2015
134879b
[tmp] Update `vim_getenv` doc
Yamakaky Aug 1, 2015
b7a56fd
[tmp] fix wrong indentation
Yamakaky Aug 1, 2015
60feda5
os/env: Add XDG_{CONFIG,DATA}_DIRS defaults
ZyX-I Jul 31, 2015
a1429b8
[tmp] fix unused var + leak
Yamakaky Aug 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/nvim/os/env.c
View file
Open in desktop
Expand Up @@ -427,6 +427,31 @@ char *vim_getenv(const char *name)
return xstrdup(kos_env_path);
}

// XDG Base Directory expansion.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function comment above needs to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (strncmp(name, "XDG_", 4) == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const char *home = vim_getenv("HOME");
if (!home) {
return NULL;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

const char *home_suffix = "";
if (strcmp(name, "XDG_DATA_HOME") == 0) {
home_suffix = "/.local/share/";
} else if (strcmp(name, "XDG_CONFIG_HOME") == 0) {
home_suffix = "/.config/";
} 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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is ~/.local/run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ~/….

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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/….

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, where are XDG_{DATA,CONFIG}_DIRS? One of the suggestions in my issue was initially populating &runtimepath from XDG_CONFIG_DIRS+XDG_CONFIG_HOME, but I think that XDG_DATA_{DIRS,HOME} should also be taken into account (reason: /usr/share/vim/vimfiles is where package managers usually put third-party plugins, thus with new directories /usr/share/nvim/site should be included and this is naturally done using XDG_DATA_DIRS, but not XDG_DATA_HOME, XDG_CONFIG_HOME or XDG_CONFIG_DIRS). This way &runtimepath will look like:

$XDG_CONFIG_HOME/nvim
$XDG_DATA_HOME/nvim
$xdg_config_dir/nvim (repeated for each directory in $XDG_CONFIG_DIRS)
$xdg_data_dir/nvim/site (repeated for each directory in $XDG_DATA_DIRS)
$VIMRUNTIME (looks something like /usr/share/nvim/runtime)
$xdg_data_dir/nvim/site/after (repeated for each directory in $XDG_DATA_DIRS)
$xdg_config_dir/nvim/after (repeated for each directory in $XDG_CONFIG_DIRS)
$XDG_CONFIG_HOME/nvim/after

XDG_DATA_DIRS defaults to /usr/local/share/:/usr/share/ according to the specification. This is why initial suggestion is using site in runtimepath only: if it was runtime then $VIMRUNTIME was naturally included (effectively meaning that this setting will become obsolete which is bad for compatibility reasons).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added commits that show how in my opinion this should look like: b4331ac, its parent and parent of its parent. Feel free to pull them (branch pr-3120 of https://github.com/ZyX_I/neovim) if you like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err. I forgot about /site: 89e1eac.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some reordering: e6c09b1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. It's just that it's a bigger change than simply using __HOME, since you can't expand XDG___DIR in macros, it needs code changes.


// We need to apprend the prefix to the HOME path.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apprend -> append

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops ^^

char *value = vim_getenv("HOME");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already have home.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yamakaky@a1429b8

value = xrealloc(value, strlen(value) + strlen(home_suffix) + 1);
strcat(value, home_suffix);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clint is warning about this strcat()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw that, on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done : 86e7c30

return value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yamakaky@a1429b8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

bool vimruntime = (strcmp(name, "VIMRUNTIME") == 0);
if (!vimruntime && strcmp(name, "VIM") != 0) {
return NULL;
Expand Down
24 changes: 16 additions & 8 deletions src/nvim/os/unix_defs.h
View file
Open in desktop
Expand Up @@ -19,6 +19,10 @@
#define SPECIAL_WILDCHAR "`'{"

// Unix system-dependent file names

#define NVIM_CONF_DIR "$XDG_CONFIG_HOME/nvim/"
#define NVIM_DATA_DIR "$XDG_DATA_HOME/nvim/"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the leading slashes should be here? Wouldn't this result in $XDG_CONFIG_HOME/nvim//... if something is appended to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter, test with cd /usr////////bin

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, that's interesting. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// does not look clean. I would avoid it when avoiding it costs nothing: just remove trailing slash here because in other places you already use leading slash.


#ifndef SYS_VIMRC_FILE
# define SYS_VIMRC_FILE "$VIM/nvimrc"
#endif
Expand All @@ -28,46 +32,50 @@
#ifndef SYNTAX_FNAME
# define SYNTAX_FNAME "$VIMRUNTIME/syntax/%s.vim"
#endif
// Not used anymore ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP. Do we also drop EXINIT ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

#ifndef USR_EXRC_FILE
# define USR_EXRC_FILE "~/.exrc"
#endif
#ifndef USR_VIMRC_FILE
# define USR_VIMRC_FILE "~/.nvimrc"
# define USR_VIMRC_FILE NVIM_CONF_DIR "/init.vim"
#endif
// Not used anymore ?
#ifndef USR_VIMRC_FILE2
# define USR_VIMRC_FILE2 "~/.nvim/nvimrc"
#endif
// Not used anymore ?
#ifndef EXRC_FILE
# define EXRC_FILE ".exrc"
#endif
// Not used anymore ?
#ifndef VIMRC_FILE
# define VIMRC_FILE ".nvimrc"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#endif
#ifndef VIMINFO_FILE
# define VIMINFO_FILE "~/.nviminfo"
# define VIMINFO_FILE NVIM_DATA_DIR "/viminfo"
#endif

// Default for 'backupdir'.
#ifndef DFLT_BDIR
# define DFLT_BDIR ".,~/tmp,~/"
# define DFLT_BDIR ".,$XDG_DATA_HOME/nvim/backup/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVIM_DATA_DIR

#endif

// Default for 'directory'.
#ifndef DFLT_DIR
# define DFLT_DIR ".,~/tmp,/var/tmp,/tmp"
# define DFLT_DIR "$XDG_DATA_HOME/nvim/swap//"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVIM_DATA_DIR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I forgot to use it for half the variables ^^

#endif

// Default for 'viewdir'.
#ifndef DFLT_VDIR
# define DFLT_VDIR "~/.nvim/view"
# define DFLT_VDIR "$XDG_DATA_HOME/nvim/view/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are not views used as configuration? I would use XDG_CONFIG_HOME here. Also this is naturally shortcuted with NVIM_{DATA,CONF}_DIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

#endif

#ifdef RUNTIME_GLOBAL
# define DFLT_RUNTIMEPATH "~/.nvim," RUNTIME_GLOBAL ",$VIMRUNTIME," \
RUNTIME_GLOBAL "/after,~/.nvim/after"
# define DFLT_RUNTIMEPATH NVIM_CONF_DIR "," RUNTIME_GLOBAL ",$VIMRUNTIME," \
RUNTIME_GLOBAL "/after," NVIM_CONF_DIR "/after"
#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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#endif // NVIM_OS_UNIX_DEFS_H