Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion bin/install_uninstall.ml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ let interpret_destdir ~destdir path =
let get_dirs context ~prefix_from_command_line ~from_command_line =
let open Fiber.O in
let module Roots = Install.Roots in
let prefix_from_command_line = Option.map ~f:Path.of_string prefix_from_command_line in
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it not work if we only use of_filename_relative_to_initial_cwd here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried that first, but it converts all relative paths to absolute, which changed the output in many tests across the codebase (paths like _install became $TESTCASE_ROOT/_install). This approach only converts paths that would escape the workspace, avoiding those test changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's not all that convincing. We should ideally have one way of converting such stings to paths in the frontend. I guess this does fix the issue though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, a unified approach would be better. Happy to refactor it later if you'd like.

let prefix_from_command_line =
Option.map ~f:Path.of_string_allow_outside_workspace prefix_from_command_line
in
let+ roots =
match prefix_from_command_line with
| None -> Memo.run (Context.roots context)
Expand Down
2 changes: 2 additions & 0 deletions doc/changes/fixed/12993.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Fix `dune install --prefix` failing with relative paths outside the workspace
like `../foo` (#12993, fixes #12241, @benodiwal)
9 changes: 9 additions & 0 deletions otherlibs/stdune/src/path.ml
Original file line number Diff line number Diff line change
Expand Up @@ -945,6 +945,15 @@ let of_filename_relative_to_initial_cwd fn =
external_ (External.of_filename_relative_to_initial_cwd fn)
;;

let of_string_allow_outside_workspace s =
if Filename.is_relative s
then (
match Local.L.relative_result Local.root (explode_path s) with
| Ok _ -> of_string s
| Error `Outside_the_workspace -> of_filename_relative_to_initial_cwd s)
else of_string s
;;

let to_absolute_filename t = Outside_build_dir.to_absolute_filename (local_or_external t)

let external_of_local x ~root =
Expand Down
5 changes: 5 additions & 0 deletions otherlibs/stdune/src/path.mli
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,11 @@ val relative_to_source_in_build_or_external
to the initial directory dune was launched in. *)
val of_filename_relative_to_initial_cwd : string -> t

(** Like [of_string], but if the path is relative and would escape the workspace
(e.g., [../foo]), it is treated as relative to the initial directory dune
was launched in instead of raising an error. *)
val of_string_allow_outside_workspace : string -> t

(** Convert a path to an absolute filename. Must be called after the workspace
root has been set. [root] is the root directory of local paths *)
val to_absolute_filename : t -> string
Expand Down
24 changes: 24 additions & 0 deletions test/blackbox-tests/test-cases/install-relative-prefix.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
Test that dune install accepts relative paths for --prefix that are outside the workspace.
This is a regression test for issue #12241.

$ cat > dune-project <<EOF
> (lang dune 3.6)
> (package (name foo))
> EOF

$ cat > dune <<EOF
> (library
> (name foo)
> (public_name foo))
> EOF

$ cat > foo.ml <<EOF
> let x = 1
> EOF

$ dune build @install

Test that relative path outside workspace works (previously failed with "path outside the workspace"):

$ dune install --prefix ../install-target --dry-run --display short 2>&1 | grep "Creating directory" | head -1
Comment thread
rgrinberg marked this conversation as resolved.
Creating directory $TESTCASE_ROOT/../install-target/lib/foo
Loading