Skip to content

Move histfile to the system's "state" folder#362

Closed
exorcist365 wants to merge 17 commits intoocaml-community:masterfrom
exorcist365:master
Closed

Move histfile to the system's "state" folder#362
exorcist365 wants to merge 17 commits intoocaml-community:masterfrom
exorcist365:master

Conversation

@exorcist365
Copy link
Copy Markdown

No description provided.

Comment thread src/lib/uTop.ml Outdated
Comment thread src/lib/uTop.ml Outdated
Comment thread src/lib/uTop.ml Outdated
@exorcist365
Copy link
Copy Markdown
Author

I'm not exactly sure how to create the directory if it doesn't exist, I'm still new to OCaml and not yet familiar with most libraries.

Comment thread src/lib/uTop.ml Outdated
Comment thread src/lib/uTop.ml Outdated
@redianthus
Copy link
Copy Markdown

redianthus commented Nov 29, 2021

I'm not sure that adding Bos as a dependency is something we want, but I'm not the one who can make the decision here.

If it's OK to have it, then, you should probably match on the result and print a warning/fail if the dir hasn't been created - and not use ignore.

If it's not, you could just reimplement the part of Bos.OS.Dir.create that is relevant here.

@pmetzger
Copy link
Copy Markdown
Member

Adding more dependencies for something very small is probably not optimal, no.

@lindig
Copy link
Copy Markdown
Contributor

lindig commented Nov 30, 2021

The code below can be used to create a directory, including parent directories. If this is what you need.

(** create a directory but doesn't raise an exception if the directory already exist *)
let mkdir_safe dir perm =
  try Unix.mkdir dir perm with Unix.Unix_error (Unix.EEXIST, _, _) -> ()

(** create a directory, and create parent if doesn't exist *)
let mkdir_rec dir perm =
  let rec p_mkdir dir =
    let p_name = Filename.dirname dir in
    if p_name <> "/" && p_name <> "."
    then p_mkdir p_name;
    mkdir_safe dir perm in
  p_mkdir dir

Comment thread src/lib/uTop.ml Outdated
Comment thread src/lib/uTop.ml Outdated
Comment thread src/lib/uTop.ml Outdated
Comment thread src/lib/uTop.ml Outdated
Comment thread src/lib/uTop.ml
@Firobe
Copy link
Copy Markdown

Firobe commented Jan 8, 2022

Thanks for making this PR ! This is a detail but shouldn't the file be in XDG_STATE_HOME instead of XDG_CACHE_HOME ? At least that's what the specification (https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html) mentions.

@redianthus
Copy link
Copy Markdown

Oh, I didn't know about it thanks ! It's been introduced in the latest basedir spec. I'll add it to directories. :)

@redianthus
Copy link
Copy Markdown

Done, should be available soon, see ocaml/opam-repository#20409.

Comment thread src/lib/uTop.ml Outdated
Comment thread utop.opam Outdated
@exorcist365 exorcist365 changed the title Move histfile to the system's cache folder Move histfile to the system's "state" folder Jan 28, 2022
@redianthus
Copy link
Copy Markdown

Thanks @exorcist365. I believe the PR can be merged but I don't have the rights to do it...

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Jan 31, 2022

Hi! I feel like adding a transitive dependency to ctypes on windows (through directories) is a bit heavy. Is there a way to have native bindings to the corresponding APIs?

@redianthus
Copy link
Copy Markdown

Note that some effort has been made not to depend on ctypes.foreign and libffi but only ctypes.stubs and ctypes. Regarding native bindings, I personally won't go into the trouble of it.

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Feb 1, 2022

Sure, I understand. To give you some context, we're making sure that a selection of packages (including utop) are available as soon as new versions of the ocaml compiler are released. Adding ctypes on windows to the mix seems difficult but I'll check with the rest of the team.

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Jul 4, 2022

It looks like xdg is going to provide this kind of support on windows without the ctypes dependency: ocaml/dune#5943
If this works in the context of this PR, I think that this would be preferable in terms of dependencies.

@redianthus
Copy link
Copy Markdown

Well, in the current state the PR doesn't look correct to me (I commented there), but if it ends up providing a proper way to get the cache directory without ctypes it's great.

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Feb 22, 2023

I'm not willing to take a dependency on ctypes at the moment, but please reopen if you're willing to implement this on top of xdg or a similar lighter alternative. thanks!

@emillon emillon closed this Feb 22, 2023
@Skyb0rg007
Copy link
Copy Markdown
Contributor

lambda-term has included support for the XDG Base Directory spec since version 3.0.0, including legacy fallback handling.
Perhaps this can be used to avoid a dependency.

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.

7 participants