[WIP] XDG support #3198

jck · 2015-08-19T09:33:38Z

This is a continuation of the discussion in #3127 .
I've addressed some of the comments in 8fc0976 but there is still quite a bit to do.

lucc · 2015-08-19T11:46:03Z

src/nvim/os/stdpaths.c

+  [cache_home] = "~/.cache",
+  [runtime_dir] = "",
+  [config_dirs] = "/etc/xdg/",
+  [data_dirs] = "/usr/local/share/:/usr/share/",


Is there a reason that some of these values have a slash at the end and some don't?

Just following the spec: http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html

jck · 2015-08-20T09:09:09Z

src/nvim/os/stdpaths.c

+  [kConfigDirs] = "/etc/xdg/",
+  [kDataDirs] = "/usr/local/share/:/usr/share/",
+};
+#endif


@pyrohh Does this look good?

I prefer explicit defines (as opposed to else), but in this case why not.
CYGWIN <-> ~ ??

@jck Looks fine to me, no comment on the actual defines though. I'm not sure if it should instead be __WIN32__ and __APPPLE__, but we'll deal with that later.

rainerborene · 2015-08-21T17:11:56Z

Make sure to update the man page as well.

splinterofchaos · 2015-08-23T18:00:32Z

src/nvim/os/stdpaths.c

+    ret = (const char *)expand_env_save((char_u *)fallback);
+  }
+
+  return ret;


This returns statically allocated memory when os_getenv(env) returns non-null, and heap-allocated memory when it returns null and fallback isn't null. How does the caller know whether the return value requires an xfree()? We could add a parameter, bool *must_free, but it may be simpler to always return allocated memory.

const char *ret = os_getenv(env); if (ret) { ret = xstrdup(ret); } else if (fallback) { // ... same as before }

^ This. This should fix the leak ASAN is complainign about.

justinmk · 2015-08-26T04:49:09Z

Hoping to get this in 0.1 milestone.

justinmk · 2015-09-22T19:17:42Z

A bit late, but in case it helps, this was mentioned in #78 as a useful reference:

https://github.com/ActiveState/appdirs/blob/master/appdirs.py

tarruda · 2015-09-24T10:55:29Z

Will this PR also allow storage of undo and swap files under the cache directory?

justinmk · 2015-09-24T13:38:14Z

@tarruda Not sure about the exact location, but it would allow us to turn those on by default, which is I think is desirable.

jck · 2015-10-02T19:54:08Z

@justinmk I'll finish this off this weekend. I hope that will be enough for 0.1

fmoralesc · 2015-10-02T20:01:58Z

src/nvim/main.c

-          && do_source((char_u *)USR_VIMRC_FILE3, TRUE,
-            DOSO_VIMRC) == FAIL
-#endif
+      if (do_source(user_vimrc, TRUE, DOSO_VIMRC) == FAIL


Most of main.c appears to be using TRUE and FALSE. Should I just fix all of them in a new commit/different PR?

No, just change these, the travis build won't pass until you do so. Merely stylistic PRs are not terribly important, and since the linter starts looking for issues in the surrounding lines, they can become quite unwieldy.

fmoralesc · 2015-10-02T20:03:36Z

src/nvim/os/stdpaths.c

+
+const char *get_from_user_data(const char * fname)
+{
+  const char *dir = (const char *)concat_fnames(get_user_data_dir(), fname, true);


clint says this line is too long

Also, there is a leak here.

Speaking of that, is there any static analysis planned ? It could be a nice addition.

We already have static analysis:

https://neovim.io/doc/reports/clang/
https://scan.coverity.com/projects/2227

However coverity has been broken for weeks.

fmoralesc · 2015-10-03T10:11:01Z

@justinmk I think this should aim for 0.1. 0.1 will probably cause quite a bunch of people to try out nvim, and I feel it's better to have them put their configuration files in the right place right away.

tarruda · 2015-10-03T11:44:46Z

src/nvim/os/unix_defs.h

@@ -31,12 +31,6 @@
 #ifndef USR_EXRC_FILE
 # define USR_EXRC_FILE "~/.exrc"
 #endif
-#ifndef USR_VIMRC_FILE
-# define USR_VIMRC_FILE "~/.nvimrc"


Does this mean that users will be forced to use the new location? Isn't it better to use the old files in case it is not found in the XDG dirs? cc @justinmk

In #78 @ZyX-I made the case that there is no reason for us to support both old and new config locations. It bothers me because it means the default location is different on each system (this makes support more difficult, and requires extra setup to share configs across systems using GitHub etc).

I would be less bothered by it if we defaulted to ~/.config on all systems (unix, windows, osx) instead of the terrible ~/Library and %APPDATA% paths. (No one really wants their configs there, do they?)

It's not your place to question it really. These are the standards set forth by the operating systems you target. I'll quote some of Raymond Chen's ironic maxims about his frustration when dealing with software during the MS-DOS to Windows 95 transition:

4. Manuals are a waste of time. 9. Find a rule and break it as blatantly as possible. 19. Second-guess the specification whenever possible.

Please don't keep repeating the same mistakes. As for support, different locations is trivial to deal with, please don't use it as an excuse to do the wrong thing.

12. It is better to be lucky than good.

@tarruda This software is pre-1.0 release (assuming you're following semver), there is no reason to support legacy. Just do the right thing now instead.

I'm with @Earnestly (and @ZyX-I) on this one.

@Earnestly Where does the standard specify the default locations for OSX and Windows? So far only source examples like https://github.com/ActiveState/appdirs/blob/master/appdirs.py have been offered, which indicate "best guesses" about default locations, there is no reference to a specification.

@kopischke I'm not a OSX user (never really used it, actually), so I was just being conservative ;)

@justinmk Taking a look at the basedir spec again, most of the XDG_ variables are defined relative to $HOME, which has its equivalent in %UserProfile% under Windows. I guess in that sense using that should be the correct behavior as per the basedir spec. However, XDG_DATA_DIRS and XDG_CONFIG_DIRS are undefined, since they default to paths Windows doesn't have.

http://standards.freedesktop.org/basedir-spec/latest/ar01s03.html

@justinmk Windows is out of scope of XDG Basedir, follow Windows' standards for Windows instead. For example, Vala's Environment.get_user_config_dir() uses CSIDL_LOCAL_APPDATA for Windows.

What you want to do about Windows however is up to you,
thank you for working on this btw. ❤

@fmoralesc nothing wrong with that, especially as OS X’ “almost, but not quite, Unix” system does need special treatment in many areas. The XDG spec as applied to CLI applications is not one of them, however, so let’s count our blessings and limit special treatment to The Odd Man Out™, aka Windows :).

@kopischke 👍

fmoralesc · 2015-10-03T17:14:49Z

I was just trying out the PR locally, and it seems to me like it still needs to handle 'runtimepath', 'undodir, etc. Also, it would be good to show the new place for the user initialization file in the output of :version. There isn't a way to query this value as far as I can tell.

ZyX-I · 2015-10-03T18:40:18Z

@fmoralesc Initialization file location should be stored in $MYVIMRC environment variable. If this is not true with this PR then it needs fixing.

fmoralesc · 2015-10-03T19:03:53Z

@ZyX-I No, it's OK in this PR, I just missed it (I think I tried it, but I must have made a typo so I didn't see it). Still, I opened a PR in @jck's repo to add it to the output of :version.

splinterofchaos · 2015-10-07T06:04:31Z

src/nvim/option.c

+  ga_init(&rtp_ga, (int)sizeof(const char *), 1);
+  GA_APPEND(const char *, &rtp_ga, get_user_conf_dir());
+  GA_APPEND(const char *, &rtp_ga, concat_fnames(get_user_conf_dir(), "after", true));
+  set_string_default("runtimepath", ga_concat_strings(&rtp_ga));


The memory allocated by concat_fnames must be xfreed. Doesn't look like we gain much by using garray_T here. sprintf() (or the custom vim variant) might be more appropriate.

Would you suggest that I precomute the length of the rtp string like this?
ZyX-I@b4331ac

The safest bet for allocating path names is just xmalloc(PATH_MAX). If you care about over-allocated memory, you can xrealloc(rtp strlen(rtp) + 1) later, but it's not necessary.

I just realized another issue: It looks like the resulting string will be "{get_user_conf_dir()}{get_user_conf_dir()}/after". Could just do this:

char* conf_dir = get_user_conf_dir(); set_string_default("runtimepath", concat_fnames(conf_dir, "after", true); xfree(conf_dir);

Relying on PATH_MAX is extremely silly and not a very well researched suggestion. Consider the implications of what you're suggesting on a portability level. No, correct handling of paths involve using calloc and snprintf. (I assume xmalloc is just a wrapper around malloc that provides overflow checking like calloc but without the guarantee of zero initialised memory.)

Unfortunately (I guess), we have a lot of code that check paths are less than the maximum, so this convention wouldn't be easy to remove from the codebase, even if it's wrong. For the moment, I think it would be correct to just say "we don't support unreasonably large paths," and if supporting them means unleashing such inefficient code such as in the linked article, I don't see any benefit.

Also, I double checked: I should have said MAXPATHL, an os-independent variable that we define.

@splinterofchaos Why not simply use my code with minimal modifications (to use different functions to get XDG_* variables)? I see that this variant uses only XDG_CONFIG_HOME which is not enough.

(Note: do not use the commit you referenced, use the head of pr-3120 branch.)

@ZyX-I “53. So many good ideas are never heard from again once they embark in a voyage on the semantic gulf.” — Alan J. Perlis

fmoralesc · 2015-10-08T08:00:03Z

'runtimepath' needs to include the system runtime path.

ZyX-I · 2015-10-17T10:13:51Z

@fmoralesc My variant includes system runtime path.

BTW, after #2506 is merged this PR needs rebasing or you will not be able to move shada file to XDG-compatible location. I have updated #78 to reflect changes (for some reason that issue did not talk about viminfo though).

fmoralesc · 2015-10-17T10:25:15Z

@ZyX-I Yes, I just pointed it out because the current version didn't. I hope @jck picks this up again, otherwise could you take over?

ghost · 2015-10-17T18:46:01Z

Closing in favor of #3470, but if I missed something please reopen.

marvim added the WIP label Aug 19, 2015

lucc reviewed Aug 19, 2015
View reviewed changes

ghost mentioned this pull request Aug 19, 2015

[WIP] XDG support #3120

Closed

jck reviewed Aug 20, 2015
View reviewed changes

splinterofchaos reviewed Aug 23, 2015
View reviewed changes

justinmk mentioned this pull request Aug 23, 2015

Newsletter vNext neovim/neovim.github.io#110

Closed

sethjackson mentioned this pull request Aug 26, 2015

[RFC] Add path definitions for Windows. #3242

Merged

justinmk added this to the 0.1-first-public-release milestone Aug 26, 2015

tarruda modified the milestones: 0.2, 0.1-first-public-release Oct 2, 2015

fmoralesc reviewed Oct 2, 2015
View reviewed changes

tarruda modified the milestones: 0.1-first-public-release, 0.2 Oct 3, 2015

tarruda reviewed Oct 3, 2015
View reviewed changes

splinterofchaos reviewed Oct 7, 2015
View reviewed changes

jck added 9 commits October 7, 2015 04:08

stdpaths

b8f07f8

vimrc

140b470

vdir

05b7db7

don't show "usr vimrc file:.." in --version

be834ff

bdir xdg

a629a05

directory xdg

fb1e6e5

undodir xdg

b47e51c

vdir

c962ec3

wip: xdg rtp

49223e7

ZyX-I mentioned this pull request Oct 17, 2015

XDG base directory specification support #3470

Merged

ghost closed this Oct 17, 2015

jszakmeister removed the WIP label Oct 17, 2015

This pull request was closed.

[WIP] XDG support #3198

[WIP] XDG support #3198

Conversation

jck commented Aug 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainerborene commented Aug 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented Aug 26, 2015

justinmk commented Sep 22, 2015

tarruda commented Sep 24, 2015

justinmk commented Sep 24, 2015

jck commented Oct 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmoralesc commented Oct 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmoralesc commented Oct 3, 2015

ZyX-I commented Oct 3, 2015

fmoralesc commented Oct 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmoralesc commented Oct 8, 2015

ZyX-I commented Oct 17, 2015

fmoralesc commented Oct 17, 2015

ghost commented Oct 17, 2015