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

RFC: feat(config): Use more conventional paths for config and data #5336

Merged
merged 4 commits into from Feb 8, 2018

Conversation

Projects
None yet
4 participants
@wbinnssmith
Copy link
Contributor

commented Feb 8, 2018

This implements:

  • Supporting user-defined environment variables adhering to the XDG
    Specification

    to override yarn's defaults
  • Supporting equivalent environment variables on Windows such as
    %LOCALAPPDATA%
  • More conventional defaults for these locations according to the
    operating system.
  • Support for the user defined config dir in the .yarnrc lookup path
  • Storing global modules in a data-oriented location rather than a
    config location (seeing this is actually what motivated this PR)

Concerns:

  • Existing Windows config locations will break. This probably need to be
    addressed with a migration path and/or a breaking change
  • A few notes included in comments (will highlight these with inline GH
    comments)
  • Unclear test status as master builds appears to fail on my laptop as
    well.

Really interested in your feedback. I know this has been attempted
before
-- cc @kelseasy
-- and I'd really like to get this in!

Will Binns-Smith
feat(config): Use more conventional paths for config and data
This implements:

* Supporting user-defined environment variables adhering to the [XDG
Specification](https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.6.html)
to override yarn's defaults
* Supporting equivalent environment variables on Windows such as
%LOCALAPPDATA%
* More conventional defaults for these locations according to the
operating system.
* Support for the user defined config dir in the `.yarnrc` lookup path
* Storing global modules in a data-oriented location rather than a
config location (seeing this is actually what motivated this PR)

Concerns:

* Existing Windows config locations will break. This probably need to be
addressed with a migration path and/or a breaking change
* A few notes included in comments (will highlight these with inline GH
comments)
* Unclear test status as master builds appears to fail on my laptop as
well.

Really interested in your feedback. I know [this has been attempted
before](https://github.com/yarnpkg/yarn/pull/3674/files) -- cc @kelseasy
-- and I'd really like to get this in!
@wbinnssmith

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2018

cc @Daniel15 I feel like you'd have some insight here, especially re: Windows paths


export function getCacheDir(): string {
if (process.platform === 'win32') {
// process.env.TEMP also exists, but most apps put caches here

This comment has been minimized.

Copy link
@wbinnssmith

wbinnssmith Feb 8, 2018

Author Contributor

@Daniel15 do you know about this?

This comment has been minimized.

Copy link
@Daniel15

Daniel15 Feb 14, 2018

Member

TEMP is for temp files (anything you don't care about any more after some period of time while it's being used), whereas I think we want to keep the Yarn cache around.

Will Binns-Smith added some commits Feb 8, 2018

Will Binns-Smith
@buildsize

This comment has been minimized.

Copy link

commented Feb 8, 2018

This change will increase the build size from 10.45 MB to 10.45 MB, an increase of 5.26 KB (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 905.49 KB 905.87 KB 395 bytes (0%)
yarn-[version].js 3.93 MB 3.94 MB 2.07 KB (0%)
yarn-legacy-[version].js 4.08 MB 4.08 MB 2.05 KB (0%)
yarn-v[version].tar.gz 910.85 KB 911.21 KB 373 bytes (0%)
yarn_[version]all.deb 672.86 KB 673.24 KB 394 bytes (0%)
@Daniel15

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

// but that feels unintuitive for a cli tool

// Instead, always use the typical location of XDG_DATA_HOME
return path.join(userHome, '.local', 'share', 'yarn');

This comment has been minimized.

Copy link
@bestander

bestander Feb 8, 2018

Member

I have a concern that this changes current setup people have, may cause confusion.

Before it was ~/.config/yarn/global, now it will be ~/.local/share/yarn/global.
Do we really need this change?

This comment has been minimized.

Copy link
@wbinnssmith

wbinnssmith Feb 8, 2018

Author Contributor

Honestly this is my main reason for the PR 😝

I thought it was really, really weird to be storing a ton of binaries in the user's configuration directory. The kind of stuff we put here is a perfect fit with XDG_DATA_HOME, which on many machines defaults to this location.

@bestander
Copy link
Member

left a comment

I like the approach to isolate the code.
But I would keep the old paths if they unless there is a stronger reason to migrate.

@wbinnssmith

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2018

But I would keep the old paths if they unless there is a stronger reason to migrate.

Yeah, I definitely understand. Personally I'd like to discuss a migration path (partly why this PR is RFC). Maybe a quick stat to see if this directory exists and either move its contents or fall back to it?

@kelseasy

This comment has been minimized.

Copy link

commented Feb 8, 2018

The idea behind the change is that it's easier for people using mounts to take advantage of the xdg spec (like having your flash drive mounted on the cache directory) to get a working setup out of the box without having to rely on creating symlinks.

The approach used in other package managers is usually to check if the legacy directory exists and use it if it does. You can also have a symlink for a few releases to give people time to adapt.

Thanks for picking this up @wbinnssmith!

@bestander

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

Looks good, let's merge it and discuss the migration path in the next PR

@bestander bestander merged commit 2d454b5 into yarnpkg:master Feb 8, 2018

9 of 10 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
buildsize/total Build size: 10.45 MB (increased by 5.26 KB, 0.05%)
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test-linux-node4 Your tests passed on CircleCI!
Details
ci/circleci: test-linux-node6 Your tests passed on CircleCI!
Details
ci/circleci: test-linux-node8 Your tests passed on CircleCI!
Details
ci/circleci: test-macos-node6 Your tests passed on CircleCI!
Details
ci/circleci: test-macos-node8 Your tests passed on CircleCI!
Details

wbinnssmith pushed a commit to wbinnssmith/yarn that referenced this pull request Feb 9, 2018

Will Binns-Smith
RFC: Make conventional directories for config and data the default
Following from yarnpkg#5336, this makes the typical directories for XDG and
Windows conventions the default.

This RFC is really carried over from yarnpkg#5336, where we continued to use
the existing locations inside ~/.config and punted on migrating users to
these new locations.

A suggestion from the prior PR include moving the contents of
the existing directories (should they exist), leaving a symlink in
place.

I really like this approach, however there will be a cost to checking to
see if these directories exist and leaving symlinks behind in a
synchronous way to maintain compatibility with how we expose constants
synchronously from a module.

Please chime in!
// Use our prior fallback. Some day this could be
// return path.join(WIN32_APPDATA_DIR, 'Config')
const WIN32_APPDATA_DIR = getLocalAppDataDir();
return WIN32_APPDATA_DIR == null ? FALLBACK_CONFIG_DIR : path.join(WIN32_APPDATA_DIR, 'Config');

This comment has been minimized.

Copy link
@Daniel15

Daniel15 Feb 14, 2018

Member

If we want to be the "most correct" here, I think the config should go in AppData, while the cache should go in LocalAppData. The difference is that LocalAppData is for anything that should not be saved to a roaming profile (ie. synchronised to a server), while regular (roaming) AppData is anything that could/should be synchronised.

Config would be something that makes sense to sync, while syncing a cache would just waste space as it could all just be redownloaded off the internet.

@severen severen referenced this pull request Aug 29, 2018

Open

XDG compliance #2334

wbinnssmith added a commit to wbinnssmith/yarn that referenced this pull request Feb 3, 2019

RFC: Make conventional directories for config and data the default
Following from yarnpkg#5336, this makes the typical directories for XDG and
Windows conventions the default.

This RFC is really carried over from yarnpkg#5336, where we continued to use
the existing locations inside ~/.config and punted on migrating users to
these new locations.

A suggestion from the prior PR include moving the contents of
the existing directories (should they exist), leaving a symlink in
place.

I really like this approach, however there will be a cost to checking to
see if these directories exist and leaving symlinks behind in a
synchronous way to maintain compatibility with how we expose constants
synchronously from a module.

Please chime in!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.