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

Eliminate dependencies on directores and dirs-sys #8048

Merged
merged 4 commits into from
Oct 21, 2024

Conversation

j178
Copy link
Contributor

@j178 j178 commented Oct 9, 2024

Summary

Migrate all directory related logic to etcetera, eliminated two dependecies.

@j178 j178 force-pushed the dir branch 4 times, most recently from 13d8b07 to df5066d Compare October 9, 2024 16:53
@j178
Copy link
Contributor Author

j178 commented Oct 9, 2024

I added a validation test in df5066d, to compare the outputs of the old and new code, and it passed successfully. This confirms that this PR keeps the directories unchanged.

#[cfg(test)]
mod tests {
    use etcetera::BaseStrategy;
    use std::path::PathBuf;

    fn dirs_before() -> (PathBuf, PathBuf, PathBuf, PathBuf) {
        let data_dir = directories::ProjectDirs::from("", "", "uv")
            .unwrap()
            .data_dir()
            .to_path_buf();

        let cache_dir = directories::ProjectDirs::from("", "", "uv")
            .unwrap()
            .cache_dir()
            .to_path_buf();

        let config_dir = {
            // On Windows, use, e.g., C:\Users\Alice\AppData\Roaming
            #[cfg(windows)]
            {
                dirs_sys::known_folder_roaming_app_data()
            }
            // On Linux and macOS, use, e.g., /home/alice/.config.
            #[cfg(not(windows))]
            {
                std::env::var_os("XDG_CONFIG_HOME")
                    .and_then(dirs_sys::is_absolute_path)
                    .or_else(|| dirs_sys::home_dir().map(|path| path.join(".config")))
            }
        };

        let home_dir = {
            #[cfg(windows)]
            {
                dirs_sys::known_folder_profile()
            }
            #[cfg(not(windows))]
            {
                dirs_sys::home_dir()
            }
        };

        (data_dir, cache_dir, config_dir.unwrap(), home_dir.unwrap())
    }

    fn dirs_after() -> (PathBuf, PathBuf, PathBuf, PathBuf) {
        let data_dir = etcetera::base_strategy::choose_native_strategy()
            .unwrap()
            .data_dir()
            .join("uv");
        let data_dir = if cfg!(windows) {
            data_dir.join("data")
        } else {
            data_dir
        };

        let cache_dir = etcetera::base_strategy::choose_native_strategy()
            .unwrap()
            .cache_dir()
            .join("uv");
        let cache_dir = if cfg!(windows) {
            cache_dir.join("cache")
        } else {
            cache_dir
        };

        let config_dir = etcetera::base_strategy::choose_base_strategy()
            .unwrap()
            .config_dir();
        let home_dir = etcetera::home_dir().unwrap();

        (data_dir, cache_dir, config_dir, home_dir)
    }

    #[test]
    fn ensure_directory_not_changed() {
        let before = dirs_before();
        let after = dirs_after();
        assert_eq!(before, after);
    }
}

@j178 j178 marked this pull request as ready for review October 9, 2024 17:20
@charliermarsh charliermarsh self-assigned this Oct 9, 2024
@charliermarsh
Copy link
Member

Thanks! I can review, I'm responsible for some of the mess.

@charliermarsh charliermarsh added the internal A refactor or improvement that is not user-facing label Oct 9, 2024
.and_then(dirs_sys::is_absolute_path)
.or_else(|| dirs_sys::home_dir().map(|path| path.join(".config")))
}
etcetera::choose_base_strategy()
Copy link
Member

Choose a reason for hiding this comment

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

I think this isn't quite the same. It looks like etcetera will use local AppData and then falls back to Roaming AppData, whereas the above always uses the Roaming version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dirs-sys uses the KnownFolderID FOLDERID_RoamingAppData, while etcetera uses the CSIDL CSIDL_APPDATA. And FOLDERID_RoamingAppData is equivalent to CSIDL_APPDATA in CSIDL.

If the SHGetFolderPathW call fails, etcetera will fall back to use self.home_dir.join("AppData").join("Roaming"). So it seems etcetera always uses the Roaming APPDATA too, they should be the same.

let home_dir = dirs_sys::home_dir();
home_dir.map(|path| path.join(".local").join("bin"))
let home_dir = etcetera::home_dir();
home_dir.map(|path| path.join(".local").join("bin")).ok()
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be mostly the same except that it defers to USERPROFILE first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's unfortunate. What's your suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we already depend on homedirs via axoupdater... So in theory we could call homedirs::my_home here at least on Windows, which does seem to use FOLDERID_Profile. Or we could just inline our own home_dir impl for Windows here, and add a TODO to move to etcetera on the next breaking release (0.5.0). Or we could just ship this whole thing in 0.5.0.

Copy link
Member

Choose a reason for hiding this comment

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

@zanieb -- I think my preference here is just to ship this as part of 0.5.0 in the near-ish future, and accept that we now respect non-empty USERPROFILE on Windows (which seems fine).

@charliermarsh charliermarsh added this to the v0.5.0 milestone Oct 16, 2024
@charliermarsh
Copy link
Member

Gonna merge into v0.5.0.

@charliermarsh charliermarsh merged commit 1653daa into astral-sh:tracking/050 Oct 21, 2024
zanieb added a commit that referenced this pull request Oct 21, 2024
Migrate all directory related logic to `etcetera`, eliminated two
dependecies.
zanieb added a commit that referenced this pull request Oct 21, 2024
Migrate all directory related logic to `etcetera`, eliminated two
dependecies.
zanieb added a commit that referenced this pull request Oct 22, 2024
Migrate all directory related logic to `etcetera`, eliminated two
dependecies.
@j178 j178 deleted the dir branch October 24, 2024 14:30
zanieb added a commit that referenced this pull request Oct 24, 2024
Migrate all directory related logic to `etcetera`, eliminated two
dependecies.
@zanieb zanieb mentioned this pull request Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal A refactor or improvement that is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants