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

Cache directory respects XDG_CACHE_HOME (basedir spec) #214

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

jasonkarns
Copy link
Contributor

@jasonkarns jasonkarns commented Mar 20, 2020

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[x] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)
Instead of setting the cacheLocation to a directory in the user $HOME, this PR now respects the XDG Basedir Specification (specifically the XDG_CACHE_HOME environment variable).

Which issue (if any) does this pull request address?
fixes standard/standard#1501

Is there anything you'd like reviewers to focus on?
The XDG Basedir spec states that XDG_CACHE_HOME should be used (if set) to determine where to write cache files. It is recommended and conventional that tools create a self-named directory beneath cache-home for the files to live in. If the XDG_CACHE_HOME is empty or unset, a value of $HOME/.cache is used. (TMPDIR is used if HOME is unset/empty) Additionally, to mirror the existing support for storing multiple caches, a subdirectory for each major version is used beneath the linter's cache directory. (${XDG_CACHE_HOME:-$HOME/.cache}/standard/vX)

xdg-basedir is a package by sindresorhus which derives the appropriate values for the different XDG directories according to the spec (and respecting the fallback values). This package derives these values only once, when required. Because the values are cached, it is non-trivial to write tests to cover the various env-var scenarios. Due to this and the simplicity of the code change, I opted to not add any tests to the existing (simple) suite.

In place of tests, here are the various scenarios covered manually:

  1. XDG_CACHE_HOME configured to a custom location:

    $ XDG_CACHE_HOME=/my/cache node -p "new require('./').linter({cmd: 'linty', eslint: require('eslint')}).eslintConfig.cacheLocation"
    /my/cache/linty/v0/
  2. XDG_CACHE_HOME is unset or empty; (uses the XDG default: $HOME/.cache/)

    $ XDG_CACHE_HOME= node -p "new require('./').linter({cmd: 'linty', eslint: require('eslint')}).eslintConfig.cacheLocation"
    /Users/jasonkarns/.cache/linty/v0/
  3. XDG_CACHE_HOME is unset or empty and HOME is unset or empty; (uses TMPDIR):

    $ TMPDIR=/tempy HOME= XDG_CACHE_HOME= node -p "new require('./').linter({cmd: 'linty', eslint: require('eslint')}).eslintConfig.cacheLocation"
    /tempy/linty/v0/

@jasonkarns
Copy link
Contributor Author

I'm unsure what is going on with the tests. I get the same test failures locally when run from master.

@TillaTheHun0
Copy link

@jasonkarns I am seeing similar on updates to #194 just now. The same tests fail for me locally, and also when I run them from the tip of master.

@toddbluhm
Copy link
Contributor

While I do really like this I am a bit concerned by xdg-basedir packages first line in the readme file:

This package is meant for Linux. You should not use XDG on macOS and Windows. Instead, you should follow their platform conventions. You can use env-paths for that.

Maybe this should be using env-paths instead? I know xdg-basedir won't break because it has non-linux os safeguards in the code, but if we are going to make the change, maybe we should make it work for all platforms by using the appropriate cache location on each platform?

@jasonkarns
Copy link
Contributor Author

jasonkarns commented Apr 30, 2020

@toddbluhm Personally, I don't agree with "You should not use XDG on macOS and Windows.". I'm on macOS and have been using XDG's locations for years.

I might agree for tools and apps that are built for non-developers. In that case, it makes sense to put data in ~/Library/** as per the apple reference guidelines. However, for developers who spend all day in Terminal, and likely have a dotfiles repo for versioning their configs, and use whole piles of *nix tools, I strongly strongly favor XDG's structure.

Windows, eh, I have no strong opinion. Though I would wager with the prevalence of git-bash, and WSL that even there, developer-focused tools should remain consistent with the XDG directories. (All the XDG defaults still live in $HOME, anyway, so XDG still works on Windows just fine.)

@toddbluhm
Copy link
Contributor

@jasonkarns Thank you for that clarification. I do agree that since this is a developer tool it makes sense to keep the location in the respective HOME directories. I just wanted to make sure that the warning on xdg-basedir was brought up and considered before moving forward.

@mightyiam you still okay with this change since you responded on the initial ticket? @LinusU thoughts?

I am thinking this will probably require a major version change since it changes the cache location for all platforms.

Copy link
Member

@mightyiam mightyiam left a comment

Choose a reason for hiding this comment

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

👍

Major change.

@@ -7,7 +7,7 @@ var os = require('os')
var path = require('path')
var pkgConf = require('pkg-conf')

var HOME_OR_TMP = os.homedir() || os.tmpdir()
var CACHE_HOME = require('xdg-basedir').cache || os.tmpdir()
Copy link
Member

Choose a reason for hiding this comment

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

index.js Outdated Show resolved Hide resolved
Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

LGTM

@toddbluhm toddbluhm mentioned this pull request May 4, 2020
Copy link
Contributor

@toddbluhm toddbluhm left a comment

Choose a reason for hiding this comment

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

Looks good. Just the one comment and a rebase and we can move forward on this.

@jasonkarns
Copy link
Contributor Author

Rebased!

@@ -38,8 +38,8 @@ function Linter (opts) {
var m = opts.version && opts.version.match(/^(\d+)\./)
var majorVersion = (m && m[1]) || '0'

// Example cache location: .standard-v12-cache/
var cacheLocation = path.join(HOME_OR_TMP, `.${this.cmd}-v${majorVersion}-cache/`)
// Example cache location: ~/.cache/standard/v12/
Copy link
Member

Choose a reason for hiding this comment

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

I suggest this comment be removed. It may be understood that that is the only possible pattern.

Suggested change
// Example cache location: ~/.cache/standard/v12/

@mightyiam
Copy link
Member

When merging, squash merge into a single commit?

@feross feross merged commit afa026f into standard:master Oct 27, 2020
@feross
Copy link
Member

feross commented Oct 27, 2020

Released as 13.0.0

@jasonkarns jasonkarns deleted the xdg-cache branch November 3, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Respect XDG basedir spec for cache
6 participants