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

fix(cache): use process.geteuid() for virtual users #355

Closed
wants to merge 4 commits into from
Closed

fix(cache): use process.geteuid() for virtual users #355

wants to merge 4 commits into from

Conversation

ST-DDT
Copy link
Contributor

@ST-DDT ST-DDT commented Oct 26, 2023

*/
function cacheDirectoryName(): string {
const { geteuid } = process;
if (geteuid === undefined) {
Copy link
Owner

Choose a reason for hiding this comment

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

I realized this might not exist on Windows.

I'm currently refactoring the tests to be lighter so I can bring back Windows. Will hold this until then.

@privatenumber privatenumber changed the title fix cache directory name for virtual users fix(cache): use process.geteuid() for virtual users Oct 27, 2023
@privatenumber privatenumber changed the base branch from develop to refactor-tests November 5, 2023 10:14
@privatenumber
Copy link
Owner

privatenumber commented Nov 5, 2023

Windows doesn't seem to support geteuid() (no user ID?) so I'm considering using node-cachedir to store cache based in the user's home directory.

@dcousens
Copy link
Contributor

dcousens commented Nov 7, 2023

What about os.userInfo().username as a fallback only on Windows?

Throws a SystemError if a user has no username or homedir.
(from https://nodejs.org/api/os.html#osuserinfooptions)

I don't know when that might happen, but you could default to a random tmpdir if it did

@privatenumber
Copy link
Owner

What's wrong with using node-cachedir?

@dcousens
Copy link
Contributor

dcousens commented Nov 7, 2023

https://github.com/LinusU/node-cachedir/blob/1d8619a927cfc6cafb4543ed3ce46bc6558be99f/index.js#L5

> sudo -u nobody node -e "console.log(path.join(os.homedir(), '.cache'))"
/.cache

Unfortunately, that isn't going to help for your "virtual user" environments (or any user where a homedir is /).

@dcousens
Copy link
Contributor

dcousens commented Nov 7, 2023

I think 2051f3e as written by @ST-DDT did a pretty good job, is cache sharing a problem on Windows?

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Nov 7, 2023

How about using "tsx-" + Math.random().toString().substring(2) => tsx-9675700890192862?

Works on all platforms and collisions are very unlikely. Or is there an implicit need to reuse the folder for the same user?

@dcousens
Copy link
Contributor

dcousens commented Nov 7, 2023

Or is there an implicit need to reuse the folder for the same user?

Yes, your proposal is equivalent to tsx --no-cache, my understanding is the cache is shared between runs

@privatenumber
Copy link
Owner

privatenumber commented Nov 7, 2023

On Linux, each user needs its own cache directory so it has the correct permissions to write to it. I'm not sure if this is a problem on Windows, but I rather keep the behavior consistent and also because the organization makes sense.

Do yall know if there's a way for me to test in the Linux virtual user environment? Hard to address it when I can't test the changes.

How about using "tsx-" + Math.random().toString().substring(2) => tsx-9675700890192862?

This defeats the point of the cache. (Module caching already happens inside Node). The cache is shared across multiple runs per user so it needs to be deterministic.

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Nov 7, 2023

I think on windows you always have a distinct username.

So Posix -> uid, Windows/Other -> username.replace(/\W+/g, '_')

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Nov 7, 2023

Do yall know if there's a way for me to test in the Linux virtual user environment? Hard to address it when I can't test the changes.

You could use a docker container for that.

@dcousens
Copy link
Contributor

dcousens commented Nov 7, 2023

a way for me to test in the Linux virtual user environment

The bug reported returned an ENOENT error from uv_os_get_passwd, which may be because /etc/passwd had been omitted from the image.

You could reproduce that in a number of ways, but if you're not careful, you might break your test system.

@privatenumber privatenumber deleted the branch privatenumber:refactor-tests November 8, 2023 09:11
@ST-DDT
Copy link
Contributor Author

ST-DDT commented Nov 8, 2023

@privatenumber Did you close this PR by accident?

@privatenumber
Copy link
Owner

Oops, looks like it closed as a side-effect of auto-deletion happening on a merged PR.

The reopen button is disabled for me—I think because it's your PR. Would you mind re-opening it?

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Nov 8, 2023

I cannot because you closed it.
Maybe you have to change the target branch to an existing one before reopening is possible.
Otherwise I can open a new PR tomorrow.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error uv_os_get_passwd returned ENOENT when updating to v3.14.0
3 participants