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

directory: ensure xdg compliance #7188

Closed
wants to merge 1 commit into from
Closed

Conversation

GovanifY
Copy link

@GovanifY GovanifY commented Dec 5, 2020


Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog (add file to changelog.d directory).
    ^ Not sure how I should add that to the changelog, any input appreciated!
  • The documentation has been updated, if necessary.

This ensures compliance with the XDG base specification.
This will need to be merged alongside the upstream ghc PR https://gitlab.haskell.org/ghc/ghc/-/merge_requests/4567 to avoid any unintended breakage.
This fixes the upstream bug https://gitlab.haskell.org/ghc/ghc/-/issues/6077

@GovanifY GovanifY force-pushed the xdg-posix branch 2 times, most recently from f401088 to 3b4980d Compare December 5, 2020 22:38
@phadej
Copy link
Collaborator

phadej commented Dec 5, 2020

Just making this change will break all cabal users.

There should be some migration story, and this change should be clearly and well communicated in advance. I'm sorry, but I cannot accept this PR.

(Also directory intepretation on XDG for windows is not optimal, but I don't remember the details).

@GovanifY
Copy link
Author

GovanifY commented Dec 5, 2020

I can add a check in this PR looking for .cabal, use it if found and otherwise default to XDG paths. This should ensure a smooth transition and could eventually be removed in a later release. Are you fine with that?

@@ -63,7 +63,7 @@ import Data.TreeDiff.Pretty (ansiWlEditExprCompact)
parseIndex :: (Monoid a, NFData a) => (FilePath -> Bool)
-> (FilePath -> B.ByteString -> IO a) -> IO a
parseIndex predicate action = do
cabalDir <- getAppUserDataDirectory "cabal"
cabalDir <- getXdgDirectory XdgData "cabal"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also not right. E.g. ~/.cabal/config is XdgConfig, and about everything else (e.g. ~/.cabal/store, ~/.cabal/packages) are probably XdgCache.

What about ~/.cabal/bin ?

Copy link
Author

@GovanifY GovanifY Dec 6, 2020

Choose a reason for hiding this comment

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

Yeah, I wasn't sure either of how cabal was processing all of its config, so I figured the fastest way to see was to draft up a PR for review. Will change the appropriate paths once validate finishes to complete.

~/.cabal/bin should be ~/.local/bin as per the systemd file system specification, no environment variable there. This one is a little bit less thought out but that's the current take on it.

@fgaz
Copy link
Member

fgaz commented Dec 6, 2020

Related: #680

@phadej

Also directory intepretation on XDG for windows is not optimal, but I don't remember the details

you may be referring to this: #1857 (comment) (and later comments)

@xeruf
Copy link

xeruf commented Apr 13, 2021

any progress here? I fought so hard to get a clean $HOME and now it is being littered by multiple new tools, including cabal...

@athas athas mentioned this pull request May 9, 2021
3 tasks
@ulysses4ever
Copy link
Collaborator

There was no activity here for a long while, and a more comprehensive solution is being developed in #7386. Should we close it?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 24, 2022

Wow, I didn't know about this one. @GovanifY, would you like to review #7386?

@xeruf, your help would be appreciated as well.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 26, 2022

Hello! Anybody here? :)

@GovanifY
Copy link
Author

Hey! Just saw your comment @Mikolaj .

I'm going to close this PR as yours definitely supersedes mine, although I'll have to pass on the review.
I don't feel nearly as qualified as I need to to review such a patch as yours, especially since I dropped this PR a long time ago.

Good luck with this though!

@GovanifY GovanifY closed this Aug 27, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Aug 27, 2022

Thank you. When it's released, let us know if you notice any problems.

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.

6 participants