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.

Sign up for GitHub

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

Jump to bottom

XDG Base Directory Compliance #431

Merged
merged 1 commit into from
Jul 3, 2023
Merged

XDG Base Directory Compliance #431

merged 1 commit into from
Jul 3, 2023

Conversation

Skyb0rg007
Copy link
Contributor

This PR adds support for the XDG Base Directory Specification by moving the ~/.utop-history and ~/.utoprc files to $XDG_CACHE_HOME/utop-history and $XDG_CONFIG_HOME/utop/utoprc respectively.
Both of these changes still support fallbacks to their original locations to not introduce a strict backwards incompatibility.

While this history file should really be saved as $XDG_STATE_HOME/utop-history, the XDG state directory is currently unsupported in the current version of lambda-term. I have filed a PR to that project here to add the functionality for downstream use here.

This PR improves on #362 by utilizing the existing dependency on lambda-term, and also updating the location of the utoprc file location.

@Skyb0rg007 Skyb0rg007 requested a review from rgrinberg as a code owner June 2, 2023 19:08
@emillon
Copy link
Collaborator

emillon commented Jun 5, 2023

Thanks. I suppose it's fine since we're already depending on lambda-term. But since #362, there's now xdg which is developed as part of dune. I think it should support the state directory. Have you considered using that directly?

@Skyb0rg007
Copy link
Contributor Author

While I did consider using xdg, it does not currently have support for legacy fallbacks.
However that feature is always possible to implement inside utop itself, rather than via lambda-term.
I'll see if I can update to use that instead, since it is more tailored for the intended use-case (and because my PR to xdg was accepted, but my PR to lambda-term is still waiting for a response).

emillon
emillon reviewed Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

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

Thanks a lot for adapting your work. some minor comments but this looks good!

@emillon
Copy link
Collaborator

emillon commented Jun 12, 2023

also, please add the dependency in the opam file as well.

@Skyb0rg007
Copy link
Contributor Author

Skyb0rg007 commented Jun 13, 2023 •
edited
Loading

The history file should live in ~/.local/state, but unfortunately the PR to xdg the adds the feature wasn't included in the latest release. I pushed a workaround that does the calculation inline, which can be removed once Xdg.state_dir is available.

@emillon
Copy link
Collaborator

emillon commented Jun 16, 2023

This will get released in dune 3.9.0 which should be out in a bit less than 2 weeks. It's unlikely that there'll be a utop release in the meantime so we can just wait for it to be available.

@emillon
Copy link
Collaborator

emillon commented Jul 3, 2023

I rebased and used the function from xdg now it's available.

@Skyb0rg007 @emillon
XDG Base Directory Compliance
1da42f8 
This uses xdg to load files in the right place.
@emillon emillon merged commit 9729963 into ocaml-community:master Jul 3, 2023
@emillon emillon mentioned this pull request Jul 3, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 3, 2023
@emillon
[new release] utop (2.13.0)
6808c3a 
CHANGES:

* Fix behavior of utop -stdin (ocaml-community/utop#434, fixes ocaml-community/utop#433, @tuohy)

* Handle bounds with `Zed.next_error` (ocaml-community/utop#442, @tmattio)

* Load files from XDG directories (the legacy paths still work). (ocaml-community/utop#431,
  @Skyb0rg007)

* Remove deprecated values `prompt_continue`, `prompt_comment`, `smart_accept`,
  `new_prompt_hooks`, `at_new_prompt` (#..., @emillon)

* Require OCaml 4.11.0 or newer. (ocaml-community/utop#444, @emillon)
@Skyb0rg007
Copy link
Contributor Author

Thanks for all your work giving feedback!

This was referenced Jul 5, 2023
Closed
Closed
@emillon emillon mentioned this pull request Jan 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emillon emillon emillon left review comments

@rgrinberg rgrinberg Awaiting requested review from rgrinberg rgrinberg is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Skyb0rg007 @emillon