-
Notifications
You must be signed in to change notification settings - Fork 194
Conform to XDG spec in the interpreter #753
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
base: dev
Are you sure you want to change the base?
Conversation
src/Compile/Options.hs
Outdated
| home <- getHomeDirectory | ||
| return (joinPath home kkbuild) | ||
| -- instead use `$XDG_CACHE_HOME/koka` if in the interpreter | ||
| getXdgDirectory XdgCache "koka" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "koka" should be kkbuild here as that includes the koka version and hash (as we cannot mix object and kki files across compiler versions and certain settings).
Is it ensured (well, somewhat implied would be enough) that XdgCache has execute permission?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't thought of that, I don't see any guarantee in the spec. To fix it I propose:
- The directory from $KOKA_ROOT is used
- If the env variable doesn't exist, it falls back to XdgCache
- If XdgCache doesn't have executable privileges, $HOME is used.
Would that be fine?
|
It looks like from the list here: https://wiki.archlinux.org/title/XDG_Base_Directory, that most programming languages are migrating to using something like or $KOKA_ROOT with a fallback on XDG_DATA_DIR. Including Haskell. |
Previously, the interpreter stored built files in the $HOME directory of the user. To conform to the XDG specification this was changed to $XDG_CACHE_HOME
6486ce0 to
3b63abc
Compare
If $KOKA_ROOT exists but is "", we probably still want to treat it as if it didn't exists, because it's a degenerate value, that users often use to clear out an environment variable.
Implemented as suggested. |
|
Use the kkbuild var instead of a string |
|
It is important to get this right. To me it is unclear what Maybe we should check the environment variables explicitly and still use the current solution if none of these are set? (as on Windows for example). Maybe we should explicitly check the directory for execute permission and issue a warning if we can't execute there. |
|
Agreed, this needs to be correct & tested before merging. @kyepskee if you don't have access to a machine for each Windows/MacOS/Linux, let us know and we can test it out. It seems Haskell only differentiates between Windows/Posix, so just two would be enough for this probably. Yeah, I meant XDG_DATA_HOME. The Haskell Docs don't explicitly say that there is a guarantee of returning a directory in all cases. But it does mention default paths for both Windows and Posix. Looking closer at the code it seems to always give it the default fallback. We should also add It is mentioned that this directory should be created with permissions
That's also a valid solution. Given the above which would you rather do? |
I kind of don't like using a hidden directory there for no good reason.
Do you mean if I suggest we
By impossible, I mean we cannot create directories with sufficient privileges.
I can test it on Linux only. Should I also add some tests? I don't know how I would make those honestly. Also, it would be simpler to skip |
Sorry, I didn't check closely - I was thinking it was the full build-directory (i.e. with the compiler version / hash) due to a previous comment from Daan on this PR, but this is just for the top-level build folder. I agree that since XdgData is already typically in a hidden directory, we don't need to nest another hidden, I think just "koka" is fine there (no kkbuild), if Daan is okay with that. However, for the fallback on
We unfortunately don't have these kinds of tests yet. I think some manual testing for this should be fine.
The original issue is to conform to |
… add $HOME as fallback
|
I've added checks for existence and executability of $XDG_DATA_HOME falling back to $HOME, although I think those are pathological cases and shouldn't even be checked probably. I've decided against creating directories inside
I've checked all three of these paths on Linux. This means that the directory won't have permissions |
2c83d90 to
00399ab
Compare
I changed the directory where built files are stored for the interpreter from $HOME to $XDG_CACHE_HOME according to the XDG specification. Solves #663