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

Simplify getGlobalCacheDir, change Windows default #384

Merged
merged 3 commits into from
Aug 23, 2019

Conversation

stkb
Copy link
Contributor

@stkb stkb commented Aug 21, 2019

Simplifies greatly the getGlobalCacheDir function by using Directory.getXdgDirectory, which does most of the work for us.

On Linux/MacOS, behavior isn't changed: will search for $XDG_CACHE_HOME, then $HOME, and will error if neither are found.*

On Windows, the default (if no xdg_cache_home exists) is changed to %LOCALAPPDATA%. This is really the "correct" place for it on Windows (rather than ~/.cache) to be a good citizen.

It no longer checks for/uses $HOME on Windows, but I'd argue that isn't necessary: if you want it the *nixy way, you can just set $xdg_cache_home instead (as I do). Should also fix #380.


*Now something I didn't quite understand was the alternative3 case in the original code.

https://github.com/spacchetti/spago/blob/13cf90cddb1e3e2b7338169d98799b6bc6a54ffe/src/Spago/GlobalCache.hs#L149-L178

At the end of the first line you have alternative₃ <|> err, but alternative3 can't fail, so the error message will never be shown?

So I don't know which outcome is intended if neither $xdg_cache_home nor $home are found: use a .spago-global-cache folder (in the project folder?), or show an error message; so I just went with the error message for now, which I've also modified, since it no longer applies to Windows.

Will have to see if this works on the CI...


Checklist:

(Will do after review)

  • Added the change to the "Unreleased" section of the changelog

@stkb
Copy link
Contributor Author

stkb commented Aug 21, 2019

Ok CI has failed; I will look into it...

@f-f
Copy link
Member

f-f commented Aug 21, 2019

Thanks @stkb! Some context and a few comments on this:

  • CI is failing on Linux and Windows because you removed the echoDebug line that is used in some fixtures (I'd say we should keep it as it helps with debugging), and on macOS because it fails to find a build plan (and this is puzzling)
  • the existence of the error predates the existence of alternative3. Nowadays we have a decent error message in there, but the reasoning for adding alternative3 was that the error was bad and here we can instead just provide a "safe" fallback and never fail.
    The reason why a "project local global cache" is considered safe is that Spago already has another "project local cache", and if for some reason we cannot access it then we cannot really perform any work (so it's ok to fail)
    So here I'd like to just get rid of the error message and use the safe alternative.
  • The reason why we're not using Directory.getXdgDirectory is that I copied this code from dhall-haskell.
    In there we had this same change in Use System.Directory to get cache dirs dhall-lang/dhall-haskell#550, and then added back this code in Build against GHC 7.10.3 dhall-lang/dhall-haskell#621, and then just got rid of the directory dependency altogether in Don't fail if $HOME environment variable is unset dhall-lang/dhall-haskell#789 as it was causing issues.
    Here I'm fine with this change since we don't have to support old GHC versions

Simplifies the getGlobalCacheDir function by using
`Directory.getXdgDirectory`, which does most of the work for us.

On Linux/MacOS, behavior isn't changed: will search for
`$XDG_CACHE_HOME`, then `$HOME`. Falls back to a `.spago-global-cache`
folder within the project folder if neither exist.

On Windows, the default (if no `XDG_CACHE_HOME` exists) is changed to
`%LOCALAPPDATA%`, which really is the "correct" place (in line with
Windows conventions) rather than `~/.cache`. This also fixes the bug
with using `%HomePath% (purescript#380).

Also removed the error message that wan't being used any more.
@f-f
Copy link
Member

f-f commented Aug 22, 2019

Addendum: it's also interesting to see how Pulp does this: they search for USERPROFILE on Windows and HOME elsewhere

@stkb
Copy link
Contributor Author

stkb commented Aug 22, 2019

Thanks @f-f for all the explanation. Have:

  • Put back the echoDebug statement
  • Made .spago-global-cache the fallback option
  • Removed the now-unused error message.
  • Added an entry to the changelog
  • Rebased on latest commit

Linux & Windows is now passing, but MacOS still failing on the build plan. BTW, I'm not sure if the stack.yaml.lock file should have been updated given the new explicit dependency on directory >= 1.3.4.0 ? (don't really know fully how Stack works 🤷‍♂)

Re: how Pulp does it: yeah %UserProfile% is the Windows $HOME equivalent and it will get you that, but the way we'll be doing it now (with LocalAppData) really is the correct way.

@f-f
Copy link
Member

f-f commented Aug 22, 2019

@stkb I'll try your branch on my Mac, one sec

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

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

I left a suggestion that should fix CI, but otherwise looks great, thanks! 🙂

stack.yaml Show resolved Hide resolved
Co-Authored-By: Fabrizio Ferrai <fabrizio.ferrai@gmail.com>
@stkb stkb merged commit 96e6c85 into purescript:master Aug 23, 2019
@stkb
Copy link
Contributor Author

stkb commented Aug 23, 2019

@f-f The build succeeded with that change so I have merged 😃. I still don't know about stack.yaml.lock: Stack isn't recreating it for me from either just a stack build or stack build --stack-yaml stack.yaml

@stkb stkb deleted the cachedir branch August 23, 2019 12:03
@f-f
Copy link
Member

f-f commented Aug 23, 2019

@stkb thanks! And no worries, if CI says it's good then it's good 😄 (I'll update the lockfile in the next commits)

Stack started creating/updating the lockfiles from version 2 on, so I suspect you're using a pre-2.0 version

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.

spago not working on windows
2 participants