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

Move user data to ~/.vscode[-variant]/user-data and automatically set root user data dir #5570

Closed
wants to merge 2 commits into from
Closed

Move user data to ~/.vscode[-variant]/user-data and automatically set root user data dir #5570

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2deae8f
Move user data to ~/.vscode[-variant]/user-data
Tyriar Apr 20, 2016
0e4b1bd
Set the config dir automatically when running via sudo
Tyriar Apr 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ global.vscodeStart = Date.now();
var app = require('electron').app;
var fs = require('fs');
var path = require('path');
var product = require('../product.json');
Copy link
Member

Choose a reason for hiding this comment

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

Isn't product.json one level more up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope

./product.json
./src/main.js

In packaged:

./resources/app/product.json
./resources/app/out/main.js


function stripComments(content) {
var regexp = /("(?:[^\\\"]*(?:\\.)?)*")|('(?:[^\\\']*(?:\\.)?)*')|(\/\*(?:\r?\n|.)*?\*\/)|(\/{2,}.*?(?:(?:\r?\n)|$))/g;
Expand Down Expand Up @@ -95,6 +96,26 @@ function getNLSConfiguration() {
return { locale: initialLocale, availableLanguages: {} };
}

function migrateUserDataDir(newDir) {
var oldDir = path.join(process.env.HOME, '.config', product.nameShort);
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to compute the old dir? Is it not much safer to use app.getPath('userData')

if (!fs.existsSync(oldDir)) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this test holds, please verify. My understanding is that Chrome might create this folder very early, so please test it.

Copy link
Member Author

Choose a reason for hiding this comment

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

app.setPath creates it, that's why the migration is done beforehand.

Copy link
Member

Choose a reason for hiding this comment

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

@Tyriar also keep in mind the note on 0.37.7 about setPath behavior (we are on 0.37.6): https://github.com/electron/electron/releases/tag/v0.37.7

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know, it should still work as setPath is done later on.

}
mkdirp(path.dirname(newDir));
fs.renameSync(oldDir, newDir);
Copy link
Member

Choose a reason for hiding this comment

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

Please note that rename does not work if source and target are on different drives. I think we need to add a lot more error handling to your code so that in case of an issue we can show something useful to the user.

Copy link
Member

Choose a reason for hiding this comment

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

We have to verify on all platforms that renaming the chrome data folder like this keeps all local storage functional!

}

function mkdirp(newDir) {
var stack = [];
while (!fs.existsSync(newDir)) {
stack.push(newDir);
newDir = path.dirname(newDir);
}
while (stack.length > 0) {
fs.mkdirSync(stack.pop());
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Please review this with https://github.com/Microsoft/vscode/blob/master/src/vs/base/node/extfs.ts#L51 there are some interesting issues with mkdirp

Copy link
Member

Choose a reason for hiding this comment

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

Note that any fs operation in the startup phase here can cause an error and this would be incredibly hard to find out and debug without error handling and fallbacks...

// Update cwd based on environment and platform
try {
if (process.platform === 'win32') {
Expand All @@ -107,11 +128,19 @@ try {
console.error(err);
}

// Set path according to being built or not
// If root user, use a root-owned directory for user data
var configDir = path.join(process.env.HOME, product.dataFolderName);
Copy link
Member

Choose a reason for hiding this comment

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

Are you really sure this is a solid cross platform way of finding out the users home directory? What if it is not defined? Please see https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/electron-main/env.ts#L98 how we get it.

if (process.env.VSCODE_DEV) {
var appData = app.getPath('appData');
app.setPath('userData', path.join(appData, 'Code-Development'));
configDir += '-dev';
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this actually? We already have this: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/electron-main/env.ts#L79

}
var userDataDir = path.join(configDir, 'user-data');
Copy link
Member

@bpasero bpasero Apr 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Suggest to make this use really no special characters and reflect what it stores. maybe just "userdata"? it contains all kind of Chromium stuff...


// Attempt migrations if userDataDir does not exist
if (!fs.existsSync(userDataDir)) {
Copy link
Member

Choose a reason for hiding this comment

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

What if the userdata dir exists but is a file and not a folder?

migrateUserDataDir(userDataDir);
Copy link
Member

Choose a reason for hiding this comment

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

Would this not mean we could potentially migrate the user folder from normal user to root? I would think we should not migrate for root...

}

app.setPath('userData', userDataDir);

// Use custom user data dir if specified, required to run as root on Linux
var args = process.argv;
Expand Down