Skip to content
This repository has been archived by the owner on Feb 2, 2021. It is now read-only.

If none of these environmental variables exist; this will crash. #345

Merged
merged 2 commits into from
May 28, 2015
Merged

If none of these environmental variables exist; this will crash. #345

merged 2 commits into from
May 28, 2015

Conversation

NathanaelA
Copy link
Contributor

Create a workable fallback.

return path.join(os.tmpdir(), tmp, "KillSwitches");
} else {
return path.join(os.tmpdir(), "KillSwitches");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this equivalent to

var tmp = process.env.SUDO_USER || process.env.USER || process.env.USERNAME || "";
return path.join(os.tmpdir(), tmp, "KillSwitches");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I just tested path.join and your understanding is correct. I had expected path join to do two slashes in the middle of the path... I'll repush a commit.

@teobugslayer
Copy link
Contributor

Hello @NathanaelA,
The issue with this code is that if you have machine with two simultaneously logged in users, it would either break due to missing write permissions, or one user would stop the running CLI process of the other user. What could be done is such case?

At any rate, in master this code is no longer used.

…g realized it will not double up the slashes for a empty string.
@NathanaelA
Copy link
Contributor Author

Are you sure it isn't running. I just installed NativeScript yesterday and this code file is present on my box...

@teobugslayer
Copy link
Contributor

@NathanaelA,

the code is instantiated only on Windows - see d1230fa#diff-dc9cf03aafbce6404bc646fee9d733c0R82

You are correct that this code can potentially fail on Windows, though I believe it is highly unlike for USERNAME to miss on Windows.

Maybe the correct fix is:

private static get killSwitchDir(): string {
        return path.join(os.tmpdir(), "KillSwitches");
    }

@NathanaelA
Copy link
Contributor Author

It is unlikely that it would now fail on windows; as username is set by the os and someone would have to actively remove it from the environment. But it might be worth doing the patch so that in the future if it is changed to be active on linux/osx it won't fail again.

@teobugslayer
Copy link
Contributor

I agree. Thank you for your effort!

teobugslayer added a commit that referenced this pull request May 28, 2015
If none of these environmental variables exist; this will crash.
@teobugslayer teobugslayer merged commit a1fa4b1 into telerik:master May 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants