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 windows cache permission #2474

Merged
merged 4 commits into from
Jul 21, 2021

Conversation

chenyukang
Copy link
Contributor

@chenyukang chenyukang commented Jul 16, 2021

Description

Fix issue #2454 , default WASMER_CACHE_DIR is not right for Windows.
Add more detailed messages for creating cache directory failure.

close #2454.

Copy link
Contributor

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! One last question before approving the PR.

lib/cache/src/filesystem.rs Show resolved Hide resolved
@Hywan Hywan self-assigned this Jul 17, 2021
@Hywan Hywan added 📦 lib-cache About wasmer-cache 🖼️ platform-windows This issue happens on Windows bug Something isn't working labels Jul 17, 2021
Comment on lines 20 to 21
Root: HKCU; Subkey: "Environment"; ValueType:string; ValueName: "WASMER_CACHE_DIR"; \
ValueData: "{app}\cache"; Flags: preservestringtype
Copy link
Member

Choose a reason for hiding this comment

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

If not provided, WASMER_CACHE_DIR will be WASMER_DIR + "/cache"

In this case, I think we should go to the value that worked before:

Suggested change
Root: HKCU; Subkey: "Environment"; ValueType:string; ValueName: "WASMER_CACHE_DIR"; \
ValueData: "{app}\cache"; Flags: preservestringtype
Root: HKCU; Subkey: "Environment"; ValueType:string; ValueName: "WASMER_CACHE_DIR"; \
ValueData: "{%USERPROFILE}\.wasmer\cache"; Flags: preservestringtype

Copy link
Contributor Author

@chenyukang chenyukang Jul 18, 2021

Choose a reason for hiding this comment

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

I think it's better not to set a register default value for it on Windows. We can keep the consistent usage style with Unix/Linux OS.

If users want to specify a cache directory, they can set an environment variable WASMER_CACHE_DIR, otherwise, env::temp_dir() will be used and it's safe for caching files.

/// Get the cache dir
pub fn get_cache_dir() -> PathBuf {
    match env::var("WASMER_CACHE_DIR") {
        Ok(dir) => {
            let mut path = PathBuf::from(dir);
            path.push(VERSION);
            path
        }
        Err(_) => {
            // We use a temporal directory for saving cache files
            let mut temp_dir = env::temp_dir();
            temp_dir.push("wasmer");
            temp_dir.push(VERSION);
            temp_dir
        }
    }
}

What do you think? @syrusakbary

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Looks great, merging!

@syrusakbary
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 21, 2021

@bors bors bot merged commit 2cd1df1 into wasmerio:master Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-cache About wasmer-cache 🖼️ platform-windows This issue happens on Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wasmer cli requires admin privilege on Windows due to cache
3 participants