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

Cache path in packages #3970

Closed
vitoyucepi opened this issue Jun 15, 2024 · 5 comments · Fixed by #3977
Closed

Cache path in packages #3970

vitoyucepi opened this issue Jun 15, 2024 · 5 comments · Fixed by #3977

Comments

@vitoyucepi
Copy link
Collaborator

Describe the bug
By default, the Ubuntu package sets the home directory for the liquidsoap user to /usr/share/liquidsoap, but the liquidsoap user does not have write access to the home directory, so it generates Error while loading cache: Sys_error("/usr/share/liquidsoap/.cache: Permission denied") error.

To Reproduce

  1. Install liquidsoap on ubuntu:22.04 using ubuntu package from ci pipeline artifacts.
  2. /tmp/main.liq
    output.dummy(blank())
  3. sudo -u liquidsoap liquidsoap /tmp/main.liq

Expected behavior

  1. Probably the cache directory should be set to /var/cache/liquidsoap with owner liquidsoap:liquidsoap and permissions 0755.
  2. Add build config option for custom cache path.
  3. Add the stdlib cache to the package. Probably replace stdlib completely with cache.
  4. Check if cache is writable/readable only once.
  5. Fail silently like python.

Version details

  • OS: ubuntu:22.04
  • Version: c46726b

Install method
Package from the github ci pipeline artifacts.

Common issues
#3949

@Moonbase59
Copy link

Good catch. I’m also voting for /var.

@vitoyucepi
Copy link
Collaborator Author

But that's only for liquidsoap user, all other users should use their own home dir.

I'm currently experimenting with the following set of changes:

  1. --- a/src/lang/term/term_cache.ml
    +++ b/src/lang/term/term_cache.ml
    @@ -25,10 +25,7 @@
                   Sys.chdir cwd;
                   Some (Filename.concat dir ".cache")
               | _ ->
    -              Some
    -                (Filename.concat
    -                   (Filename.concat (Unix.getenv "HOME") ".cache")
    -                   "liquidsoap")
    +              Some "/var/cache/liquidsoap"
           with Not_found -> None)
    
     let cache_dir () =
  2. pkgusers="liquidsoap"
    pkggroups="liquidsoap"
    
    # cut
    
    install -dm0755 -o liquidsoap -g liquidsoap "$pkgdir/var/cache/liquidsoap"
  3. VOLUME [ "/var/cache/liquidsoap" ]

@vitoyucepi vitoyucepi changed the title Cache path in ubuntu package Cache path in packages Jun 16, 2024
@toots
Copy link
Member

toots commented Jun 20, 2024

Thanks for reporting. I initially used https://github.com/ocamlpro/directories but then switched to $HOME for simplicity and to prevent having another external dependency.

The code for cache dir, though, is:

  (** $XDG_CACHE_HOME or $HOME/.cache *)
  let cache_dir =
    match getenvdir "XDG_CACHE_HOME" with
    | None -> option_map (fun dir -> dir / ".cache") home_dir
    | Some _dir as dir -> dir

So, not much help either.

I think that the right logic shouold be to include the cache dir in the posix build target the same way we do for other paths. For those builds, we expect a system-wide install and will use /var/cache/liquidsoap

@vitoyucepi
Copy link
Collaborator Author

If the user is not liquidsoap and the build type is posix, then the cache path should be set to

  1. $LIQ_CACHE_DIR, if it's set.
  2. $XDG_CACHE_DIR/liquidsoap if $XDG_CACHE_DIR is set.
  3. $HOME/.cache/liquidsoap.

I think debian and ubuntu packages should have a prebuilt stdlib cache, but I'm not sure about the alpine package. I'd prefer a separate package with cache, like it's done with python packages in alpine official repos.

@toots
Copy link
Member

toots commented Jun 21, 2024

Yes, there are two caches indeed, on for stdlib that should be shared and one for user scripts. I'll work on cleaning that up. Also, feel free to expand for the PR I started if you want.

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 a pull request may close this issue.

3 participants