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

Use XDG directories #3456

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use XDG directories #3456

wants to merge 2 commits into from

Conversation

adamsmd
Copy link

@adamsmd adamsmd commented Sep 24, 2016

Support for XDG directories was added in version 1.2.3.0 of the directory package so adding support to Idris was a one line change. I also removed some unneeded import lines for System.Directory that I found in the process.

This means that on Unix systems ~/.config/idris will be used instead of ~/.idris. If you have anything in ~/.idris that you want to keep using, you will have to manually move it to ~/.config/idris. The old ~/.idris can be deleted.

On Windows, the behavior is unchanged and uses %APPDATA% though I don't have a Windows machine on which to test this.

I don't know what happens on OSX, but I suspect it is the same as on Unix systems though I don't have an OSX machine on which to test this. One could make the argument that it should go to ~/Library/ or ~/Library/Preferences/ instead, but it wasn't going there before so I'll leave that for future work.

@Melvar
Copy link
Collaborator

Melvar commented Sep 24, 2016

The problem here is that not all of it is actually configuration.

@jfdm
Copy link
Contributor

jfdm commented Sep 24, 2016

@adamsmd Thanks for the contribution. Is this a fix for the issue #3334 and/or #3355? There was some discussion on potential issues affecting a change.

Unfortunately it is not a one line fix, some more parts of the compiler need changing, as @Melvar mentioned, the directory specification used by Idris is not solely contained in Idris.Info. This module exports information obtained from IRTS.System. If we are to change Idris' use of directories the changes you have suggested need to be affected there too. We may also need to double check how such a change would affect the testing scripts and the contents of Setup.hs

If we can address these then potentially we discuss if we should change where Idris installs itself.

Any questions please ask.

@adamsmd
Copy link
Author

adamsmd commented Sep 24, 2016

(Apologies for the length of reply. I just want to lay out my reasoning. I hope this doesn't devolve into a long discussion.)

@jfdm @Melvar This is not a fix of issue #3334 or #3355. Those deal install locations and are a broader issue that I don't have time to get embroiled in. This patch deals with user specific data and settings that are currently in ~/.idris. It would be a fix for #3148.

As to where things should go, from the looks of the source code, the only files in ~/.idris (and the only files effected by this patch) are repl/init and repl/history. I think repl/init is configuration so the patch is correct for that.

For repl/history it is less clear. XDG gives us only four choices: configuration (~/.config), user data (~/.local/share), cache (~/.cache), and runtime (which has no default). Which it belongs to is debatable, but here is my reasoning.

Runtime is for things like open pipes and such, so I don't think it belongs there.

At first, it might look like cache because it is automatically generated and will be regenerated if deleted. However, I would expect things in cache to be okay to put in a tmpfs that gets deleted every reboot, and history should stay around longer than that. So not cache.

This leaves configuration versus data files. This is harder to decide, and I could find no clarifications or guidelines for which history belongs to with XDG. I did find a proposal for adding a directory for application state (e.g. history, recently opened files, window locations, etc.). See https://wiki.debian.org/XDGBaseDirectorySpecification, but it doesn't look like it got traction.

If we analogize to system-level directories, I don't think history would belong in either /etc or /usr/share. The closest would be /var/lib. So that doesn't help.

I also haven't been able to figure out what other apps do for this. Nothing stood out when I looked through my ~/.config and ~/.local/share.

Is it a data file? On the one hand, history is data created by the user (which would argue for ~/.local/share), but on the other maybe it isn't critical backup information (e.g., like a saved game file) (which would argue against ~/.local/share).

Is it a configuration file? On the one hand, many apps put 'clear history' in the preferences dialog and history files are often put in ~/.<app-name> folders (which would argue for ~/.config), but on the other users usually wouldn't explicitly create or edit history (which would argue against both ~/.config and ~/.local/share depending on how you look at it).

In summary, if you think it should go in ~/.config, then this patch does that. If you think it should go in ~/.local/share, then this patch would need to be updated. In the absence of a clear reason to put it else where, I learn towards ~/.config as it keeps things together, but it is a very close call.

@adamsmd
Copy link
Author

adamsmd commented Sep 24, 2016

I don't understand the Travis output. Could someone more familiar with the system help me figure out what went wrong?

@melted
Copy link
Contributor

melted commented Sep 24, 2016

Seems like Setup.hs is flaking out with a segfault on GHC 7.10.3, maybe you can run it locally with 7.10.3 and see if it is reproducible.

@Melvar
Copy link
Collaborator

Melvar commented Sep 25, 2016

I personally would consider history, and more generally application state, to fall under user data.

@jfdm
Copy link
Contributor

jfdm commented Sep 28, 2016

It would be a fix for #3148.

First, thanks for the detail thought process. It makes you intentions and rationale nice and clear. Kudos.

From appearances, I think everyone is happy with having Idris support the XDG specification, less so about the value. I too have been considering this for alternate reasons, and had yet to think about the REPL user files.

My initial thinking is for user directory to be treated as configuration files, and for the history to be either treated as cache or data.

For the purposes of this commit, I think it is okay for Idris' REPL files to be contained within the same directory under Idris' XDG_CONFIG value. We can always fix the location, and Idris' XDG specification support at a later dat.

@jfdm
Copy link
Contributor

jfdm commented Sep 28, 2016

Also, i've restarted the trivia build, and can confirm that on Mac OS X XDG is treated as UNIX,

@jfdm
Copy link
Contributor

jfdm commented Sep 30, 2016

I've cleaned the cache and restarted the build hopefully, this will get the tests completing...

@adamsmd
Copy link
Author

adamsmd commented Oct 2, 2016

The consensus seems to be for putting repl/history in XDG_DATA (i.e. .local/share) so I have modified the commit to do that. (There was a conflict with the latest master anyway.) The result is slightly more complicated than before.

@jfdm: It looks like you've been doing some work in this area. If want to implement this yourself, feel free to do that instead of this pull request.

@ahmadsalim
Copy link

I have tried to restart the Travis jobs for 7.10.
I am unsure why the test script experiences a segmentation fault for 7.10 in particular.

@ahmadsalim
Copy link

It seems that cabal configure is having a segmentation fault on GHC version 7.10 (cabal version 1.22). Does anyone have an idea how to fix that?

@ahmadsalim
Copy link

@RyanGlScott Hi, since you are a bit familiar with GHC, I will try to ask you a bit for help! Do you have an idea why GHC 7.10.3 segfaults when increasing the directory bound? Thanks!

@RyanGlScott
Copy link
Contributor

I have no idea why it's segfaulting, and moreover, I can't reproduce it locally.

Perhaps using a more recent version of cabal-install would help?

@ahmadsalim
Copy link

@RyanGlScott Thanks, I will try.

@ahmadsalim
Copy link

It still fails even when changing cabal-install to 1.24. I have no idea why,

@RyanGlScott
Copy link
Contributor

Well, the good news is that I managed to reproduce the issue and minimize it (see my cabal-gh4367 repo). The bad news is that I have no idea what's causing this either. I've opened haskell/cabal#4423 to hopefully get to the bottom of this.

@ahmadsalim
Copy link

@RyanGlScott Thanks for taking a look! Yeah it looks very weird.

@RyanGlScott
Copy link
Contributor

As noted here, the issue seems to be that we're now installing a more recent version of process than what is shipped with GHC 7.10. My guess is that the Cabal library, which is built against the older version of process, gets confused when you compile a program that also links against the newer version of process.

A somewhat feasible workaround is to install a more recent version of Cabal alongside process. That way, when you compile the Setup.hs script, you're guaranteeing that only one version of process is used. (Experimentally, this works for me using GHC 7.10, and it would probably work with other GHCs as well.)

@ahmadsalim
Copy link

@RyanGlScott Great, thanks for looking at it. I guess this would be solved if we could somehow fix #3708 .

@mixedCase
Copy link

#3708 has been closed. Anything else blocking this?

@ahmadsalim
Copy link

@mixedCase The code does not pass the tests for 7.10.

@ahmadsalim
Copy link

I will try to increase the cabal bounds.

@ahmadsalim
Copy link

@RyanGlScott Sorry, but could you kindly try to take a look at this again?
I tried increasing the Cabal bound on travis and it did not work for me.

Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Sorry, I've lost context on this PR a bit. What is the minimum version of GHC that Idris currently supports? This matters since getXdgDirectory was introduced in process-1.2.3.0, so in order to avoid the problems in haskell/cabal#4423, I believe the simplest fix would be to use GHCs that all come shipped with a recent-enough version of process.

It appears that GHC 7.10.3 comes shipped with process.1.2.3.0, and if I'm reading this .travis.yml file correctly, then Idris is using 7.10.3 as a minimum? If so, that might make our lives easier.

@@ -264,7 +264,7 @@ Library
, cheapskate < 0.2
, containers >= 0.5 && < 0.6
, deepseq < 1.5
, directory >= 1.2.2.0 && < 1.2.3.0 || > 1.2.3.0
, directory > 1.2.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This bound should be directory >= 1.2.3, if I understand the situation correctly.

@RyanGlScott
Copy link
Contributor

Oh rats, I misread that. getXdgDirectory comes from directory, not process. Hrm...

@RyanGlScott
Copy link
Contributor

In that case, we could either:

  • Wait until Idris supports GHC 8 as a minimum
  • Attempt to backport getXdgDirectory for those who still use GHC 7.10

@ahmadsalim
Copy link

@RyanGlScott Thanks for responding.
I guess I will leave it open til GHC 8 becomes minimum then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants