Not_found exception thrown in Camomile/internal/database.ml

[rwmjones]

ocaml-gettext uses camomile. However the latest version of Camomile doesn't work - a Not_found exception gets thrown deep inside camomile.

An easy reproduce in ocaml-gettext is to simply run:

OCAMLRUNPARAM=b ../_build/bin/ocaml-gettext --action compile --compile-output fr.mo fr.po
Fatal error: exception Not_found
Raised at file "Camomile/internal/database.ml", line 50, characters 52-67
Called from file "Camomile/internal/unidata.ml" (inlined), line 187, characters 2-46
Called from file "Camomile/public/uCharInfo.ml", line 307, characters 2-30
Called from file "camomileLibrary.mlp", line 204, characters 22-44
Called from file "camomileLibrary.mlp", line 168, characters 0-1023
Called from file "Camomile/camomileLibraryDefault.ml", line 37, characters 18-46

[rwmjones]

let read dir suffix reader key =
  let fname = escape key in
  let path = Filename.concat dir (fname ^ "." ^ suffix) in
  let c = try open_in_bin path with Sys_error _  -> raise Not_found in (** here **)
  let v = reader c in
  close_in c;
  v

I can see in the code where it's thrown, but not why.

[rwmjones]

Looking at it a bit more, I see this is some kind of configuration/installation problem:

20282 openat(AT_FDCWD, "/usr/share/camomile/database/general_category.mar", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)

$ ll /usr/share/camomile/database/general_category.mar
lrwxrwxrwx. 1 root root 61 Nov  8 17:17 /usr/share/camomile/database/general_category.mar -> ../../../../../default/Camomile/database/general_category.mar

Damn, I wish people wouldn't use jbuilder ...

[rwmjones]

OK I fixed it by dereferencing the symlinks instead of copying them. Could we improve the error here? If the database files don't exist it shouldn't throw Not_found, it would be better if it failed hard with an informative error.

[rgrinberg]

Note that some code paths rely on actually catching this Not_found error. So we better be careful about changing this.

[yoriyuki]

Ok, I see four problems:

1. Make error messages more informative. (we can arbitrary define exceptions but if there is a standard way then it's better)
2. somehow symlinks don't work. why?
3. jbuilder symlinks not copies the data files. Data files are just marshalled data, so it could be potentially dangerous.
4. ocaml.gettext needs needs to read general_category.mar, which is most likely useless for this app. In future (v 2.0?), we need to break up Camomile to smaller packages.

[rgrinberg]

Yoriyuki Yamagata <notifications@github.com> writes:

1. Make error messages more informative. (we can arbitrary define exceptions but if there is a standard way then it's better)

One thing to keep in mind here is whether this exception should be catchable by the user. If we don't expect the user to catch this error, which is what seems to be the case here. Then a Not_found is definitely not adequate and a failwith with a good error message will do. A specialized exception is also good of course in either case of course.

3. jbuilder symlinks not copies the data files. Data files are just marshalled data, so it could be potentially dangerous.

I wonder if this is an issue with opam-installer. Jbuilder will indeed prefer symlinking to copying in its own internal build directories, but here the installed path itself is a symlink: /usr/share/camomile/database/general_category.mar Shouldn't opam-installer resolve these symlinks before copying?

[rwmjones]

2. somehow symlinks don't work. why?
3. jbuilder symlinks not copies the data files. Data files are just marshalled data, so it could be potentially dangerous

What was happening is that we don't have opam-installer and jbuilder can't install without it, so I was just copying the symlinks, but didn't use -L so it copied the symlinks rather than the files.

However there is still a problem with camomile:

  let c = try open_in_bin path with Sys_error _  -> raise Not_found in

If that open_in_bin function fails for any reason other than file not found (eg. broken symlink, I/O error) then the true error is hidden. The check should be specifically for file not found (mapped to Not_found), any other error should be rethrown verbatim or possibly exit hard.