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): use home::home_dir instead of HOME env. var #9

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

ErichDonGubler
Copy link
Contributor

ocrs currently relies on checking the HOME environment variable to determine where to placed cached models. Windows doesn't use this, though, so of course, it breaks. 😅

Implement the proper cross-platform logic for getting a home directory with home::home_dir.

ocrs' current dependency tree use windows 0.48.0 because of the ring dependency via dependency on ureq. This PR unfortuantely also brings in windows 0.52.0 as a duplicate dependency. briansmith/ring#1852 upgrades ring to use windows 0.52.0, and should be followed up with to remove this dep. duplication.

`ocrs` currently relies on checking the `HOME` environment variable to
determine where to placed cached models. Windows doesn't use this,
though, so of course, it breaks. 😅

Implement the proper cross-platform logic for getting a home directory
with [`home::home_dir`].

`ocrs`' current dependency tree use `windows` 0.48.0 because of the
`ring` dependency via dependency on `ureq`. This PR unfortuantely also
brings in `windows` 0.52.0 as a duplicate dependency.
[`briansmith/ring`#1852] upgrades `ring` to use `windows` 0.52.0, and
should be followed up with to remove this dep. duplication.

[`briansmith/ring`#1852]: briansmith/ring#1852
[`home::home_dir`]: https://docs.rs/home/0.5.9/home/fn.home_dir.html
@ErichDonGubler
Copy link
Contributor Author

Should resolve #3 for basic use cases. I haven't explored running Windows with many command-line arguments, but this seems to be the only blocker I could find in my basic testing.

@ErichDonGubler
Copy link
Contributor Author

Side note: the XDG specification for cache directories is a fantastic piece of ecosystem for the Linux/MacOS world. In Windows, however, there's an entirely different native convention for this. There's yet another crate and API that abstract over this for you, if you wish, called directories::ProjectDirs::data_local_dir.

@ErichDonGubler
Copy link
Contributor Author

ErichDonGubler commented Jan 3, 2024

RE: using ProjectDirs::data_local_dir vs. using $HOME/.cache/… (which, N.B., is not actually the same as ProjectDirs::cache_dir, either, since MacOS technically has a separate convention): The XDG Base Directory Specification suggests that XDG_CACHE_HOME only be used for non-essential files:

There is a single base directory relative to which user-specific non-essential (cached) data should be written. This directory is defined by the environment variable $XDG_CACHE_HOME.

The XDG_DATA_HOME seems like a more appropriate fit, since without models, inference can't even run for ocrs:

There is a single base directory relative to which user-specific data files should be written. This directory is defined by the environment variable $XDG_DATA_HOME.

@robertknight
Copy link
Owner

Thanks for this. I'll get this merged so at least the project can build. Does the use of a leading . in .cache not cause issues on Windows? I thought that might be a problem from my vague recollection of Windows file naming rules.

The current use of ~/.cache originates from this being the directory where various Python frameworks (eg Hugging Face) store their models. I hadn't looked into it more deeply than that.

@robertknight robertknight merged commit 51a4601 into robertknight:main Jan 3, 2024
1 check passed
@ErichDonGubler ErichDonGubler deleted the windows branch January 3, 2024 20:59
@ErichDonGubler
Copy link
Contributor Author

Does the use of a leading . in .cache not cause issues on Windows? I thought that might be a problem from my vague recollection of Windows file naming rules.

Not at all! Just don't expect it to be hidden by convention, like in Unix-based systems. That's a separate file attribute in Windows file systems. :-)

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