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

replace XDG Directory configuration to standard os.UserConfigDir #2826

Closed
wants to merge 1 commit into from

Conversation

aca
Copy link
Contributor

@aca aca commented Oct 23, 2019

1. Which version of Caddy are you using (caddy -version)?

v2

2. What are you trying to do?

Remove xdg config directory for universal support.

v2 introduces XDG specification.
While it is popular in linux, It's only for linux.
I don't think we need non-standard, complex third party packages for this.
But as go 1.13 now has os.UserConfigDir I think we should use it.
If we simply use os.UserConfigDir, we can also just remove homedir.(#2770)

os.UserConfigDir uses XDG_CONFIG_HOME instead of XDG_CONFIG_DATA for linux.
But it seems quite common. golang/go#29960 (comment)

And getting current directory using "." is too fragile process. Replaced it with proper function.

3. Which documentation changes (if any) need to be made because of this PR?

cancel xdg configuration

4. Checklist

  • [o] I have written tests and verified that they fail without my change
  • [o] I have squashed any insignificant commits
  • [o] This change has comments explaining package types, values, functions, and non-obvious lines of code
  • [o] I am willing to help maintain this change if there are issues with it later

@mholt
Copy link
Member

mholt commented Oct 25, 2019

Thanks for the PR, but I think this needs some more discussion first.

We've had a lot of feature requests for XDG though (#847 is just some of them). If you don't want to use it, you don't have to use it. I do not think it is harmful to support it by default.

I don't think we need non-standard, complex third party packages for this.

Which "non-standard, complex third party packages" are we using for it?

But as go 1.13 now has os.UserConfigDir I think we should use it.

The problem with this is that this function returns the user's config directory, not their data directory:

On Unix systems, it returns $XDG_CONFIG_HOME as specified by https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html if non-empty, else $HOME/.config. On Darwin, it returns $HOME/Library/Application Support. On Windows, it returns %AppData%. On Plan 9, it returns $home/lib.

But we need the data directory, $XDG_DATA_DIR.

And getting current directory using "." is too fragile process.

I don't understand why that is fragile.

@aca
Copy link
Contributor Author

aca commented Oct 26, 2019

Obviously, there was not enough explanation.

Which "non-standard, complex third party packages" are we using for it?

We've had a lot of feature requests for XDG though (#847 is just some of them). If you don't want to use it, you don't have to use it. I do not think it is harmful to support it by default.

I also think it is good to support it by default. Only problem with XDG base directory is that is absolutely Linux specific. What I meant is that to support this stuffs in more robust way, we might need to implement / use third party libraies like this. https://github.com/shibukawa/configdir. As most reliable go standard library is provided to fix this, I think It would be better to use it.

Mac and Windows don't obey XDG, so they only have one persistent per-user directory
Even on Linux, most apps store all their data in their config dir. For example: Chromium, Libreoffice, GIMP, and gcloud all seem to ignore XDG_DATA_HOME
Most programs only need one persistent per-user directory, and it's often for config files; see the links above for examples

golang/go#29960 explains about, XDG_DATA_HOME vs XDG_CONFIG_HOME

And getting current directory using "." is too fragile process.

I shouldn't have said that as I can't explain exact reason for this.
https://github.com/golang/go/blob/5d000a8b6268c09697c64c76bade1daa86f43a9e/src/os/getwd.go#L27
But when I looked at how standard library implement "get current directory". It seems It is much more complicated than just relying on '.' variable.

@aca
Copy link
Contributor Author

aca commented Oct 31, 2019

@mholt any more thoughts with this?

@mholt
Copy link
Member

mholt commented Nov 4, 2019

@aca Sort of, I still need to think about it more...

I also think it is good to support it by default. Only problem with XDG base directory is that is absolutely Linux specific. What I meant is that to support this stuffs in more robust way, we might need to implement / use third party libraies like this. https://github.com/shibukawa/configdir. As most reliable go standard library is provided to fix this, I think It would be better to use it.

It is kind of nice to have everything in one place no matter the OS -- makes troubleshooting and documentation easier. Plus, I suppose it is more likely that the process will have write permissions to the $HOME folder than other paths in the file system. But I can also see the benefit of adopting an OS's usual storage paths...

I shouldn't have said that as I can't explain exact reason for this.
https://github.com/golang/go/blob/5d000a8b6268c09697c64c76bade1daa86f43a9e/src/os/getwd.go#L27
But when I looked at how standard library implement "get current directory". It seems It is much more complicated than just relying on '.' variable.

Ah, that's because that function returns the literal, expanded path of the current directory. Whereas using . simply tells the OS to expand it for us. We don't actually need to know what the current directory is; we just need to make paths relative to the current directory and we can let the OS do the expansion. So that's the difference. Using . is fine for our needs.

@mholt
Copy link
Member

mholt commented Nov 6, 2019

Going to close this for now since I don't think it's quite the right solution (I'm still not convinced it's important to change what we have, as it's not really broken AFAIK).

@mholt mholt closed this Nov 6, 2019
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