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

windows: use ENV & SHGetFolderPathW for APPDATA/LOCALAPPDATA #11

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

utkarshgupta137
Copy link
Collaborator

It was pointed out here that etcetera simply concats the default location for Roaming & Local AppData instead of consulting the system.

Based on it, I've changed the behavior to the following:

  • Check if APPDATA & LOCALAPPDATA environment variables are defined & use them.
    This is what Go does.
  • (On Windows) use SHGetFolderPathW API (similar to how it is used in the home crate) to get these locations.
    This is what the dirs crate does.
  • Finally, if the above yield None, only then fallback to the default locations.
    This is what we do right now

This does add a new dependency, but it is already used by the home crate so it doesn't actually increase the crate size.

Copy link
Owner

@lunacookies lunacookies left a comment

Choose a reason for hiding this comment

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

This looks perfect! I don’t know anything about Windows so I really appreciate this :)

Side note, since the project has some unsafe code now, I think it would be good practice to add Miri CI (though I don’t know if accessing the Windows API while running in Miri is possible).

@utkarshgupta137
Copy link
Collaborator Author

I've tested this on my Windows machine & the tests are passing there too. Let me just test a couple of more things, & then I'll merge this.
The unsafe code is directly borrowed from the home crate, so I'm a bit confident about it.

@utkarshgupta137 utkarshgupta137 mentioned this pull request Apr 26, 2023
@lunacookies
Copy link
Owner

The unsafe code looks correct to me. In my experience writing correct unsafe Rust is impossible without Miri – the rules around provenance are too difficult for a human to verify, so you have to rely on Miri to check them empirically for you. Writing unsafe code in C (using a compiler without aggressive optimizations enabled!) is much easier than unsafe Rust, IMO.

Anyways, feel free to merge whenever you feel comfortable.

@utkarshgupta137 utkarshgupta137 merged commit a11887d into lunacookies:master Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants