Do not use opam-installer to copy files #941
Conversation
ghost
requested review from
ghost
added this to the 
bin/main.ml
Outdated
| Io.copy_file () ~src ~dst | ||
| ~chmod:(match section with | ||
| | Libexec -> (fun x -> x lor 0o111) | ||
| | _ -> (fun x -> x)) |
There was a problem hiding this comment.
opam-installer will clear the executable bit in the other cases, so it should be something like x land 0o666. (I'd recommend extracting set_executable_bits and clear_executable_bits function as well)
There was a problem hiding this comment.
thanks for the pointer, I realized we also need to add Lib_root and Libexec_root. I updated the code
| ; man = Path.relative destdir "man" | ||
| ; lib = Path.relative libdir package | ||
| ; libexec = Path.relative libdir package | ||
| ; share = Path.relative destdir ("share/" ^ package) |
There was a problem hiding this comment.
Is it OK to concatenate paths like this? I guess package is normalized already (so it does not contain slashes or ".."), but also does it work on windows?
There was a problem hiding this comment.
There might be an issue here. However we were doing the same before and we have had no issue so far. I guess we could add a few more constraints on package names
bin/main.ml
Outdated
| try | ||
| f () | ||
| with Unix.Unix_error (e, _, _) -> | ||
| Format.fprintf err_ppf "@{<error>Error@}: %s@." |
There was a problem hiding this comment.
Out of curiosity, why is it not possible to print on stderr directly?
There was a problem hiding this comment.
actually, yes it's possible. I didn't remember stderr was configured to interpret styles
There was a problem hiding this comment.
Isn't this better to do it this way so that buffering is consistent?
There was a problem hiding this comment.
This is a synchronous call so it's not an issue
|
Could it be made to use |
|
we should do it in this PR if |
bin/main.ml
Outdated
| if what = "install" then begin | ||
| Printf.eprintf "Installing %s as %s\n%!" | ||
| (Path.to_string src) | ||
| (Path.to_string_maybe_quoted dst); |
There was a problem hiding this comment.
Just curious why we're printing paths differently here (not quoted vs maybe quoted)
There was a problem hiding this comment.
Actually I didn't mean to print src as the lines are quite long already, it was just temporarily to debug something
bin/main.ml
Outdated
| files_deleted_in := Path.Set.add !files_deleted_in dir; | ||
| end; | ||
| Path.Set.to_list !files_deleted_in | ||
| |> List.rev |
There was a problem hiding this comment.
What's the significance of doing it in this order?
There was a problem hiding this comment.
It is to make sure we delete sub-directories first. I added a comment
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
opam is currently not mandatory to use dune, however one still needs the external
opam-installerbinary in order to rundune installordune uninstall. This is annoying, especially since this tool is quite simple.In order to make dune more self-contained, this PR reimplements
dune installanddune uninstallwithout calling out toopam-installer.