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

Symbolic Paths #744

Closed
wants to merge 87 commits into from
Closed
Show file tree
Hide file tree
Changes from 71 commits
Commits
Show all changes
87 commits
Select commit Hold shift + click to select a range
da56fda
WIP
rgrinberg May 4, 2018
9ba8715
Error checking for .aliases path
rgrinberg May 8, 2018
80817b4
Path.pp_debug
rgrinberg May 8, 2018
6e2d5bb
Unify build dir constants
rgrinberg May 9, 2018
f8aa2d4
Remove hard coded _build in error checking
rgrinberg May 11, 2018
99dc7e8
Change is_local to work like is_managed
rgrinberg May 11, 2018
9b4ad5e
Let the base build dir be Kind.t
rgrinberg May 12, 2018
bf44bfa
Add Path.mkdir_p
rgrinberg May 13, 2018
6e0025e
Change local_mkdirs to work on Path.Set.t
rgrinberg May 13, 2018
dc9891e
fix drop_build_dir for external paths
rgrinberg May 13, 2018
d0020b9
Fix Parh.relative to recognize absolute paths
rgrinberg May 13, 2018
9c672ba
Remove error checks that don't work for external paths
rgrinberg May 13, 2018
ec54812
Update tests
rgrinberg May 14, 2018
0900122
Make the build dir customizeable via Path.set_build_dir
rgrinberg May 14, 2018
1238861
Introduce Path.in_source
rgrinberg May 14, 2018
9282f30
Remove build dir check in_source paths
rgrinberg May 14, 2018
b3090a5
Use absolute paths for config
rgrinberg May 14, 2018
c8de924
Make the build dir configurable
rgrinberg May 14, 2018
03014ed
Fix bootstrap to set build dir
rgrinberg May 14, 2018
2efef26
Fix utop
rgrinberg May 14, 2018
dbe6453
Don't use Path.of_string where it isn't required
rgrinberg May 14, 2018
4ba811b
Set build dir in path unit tests
rgrinberg May 14, 2018
355601a
Fix Path.reach for local paths being reach from absolute paths
rgrinberg May 14, 2018
b10677a
Add tests for custom build-dir
rgrinberg May 14, 2018
9ebdac3
Fix make_local_parent_dirs
rgrinberg May 14, 2018
01f6470
Some formatting fixes
rgrinberg May 14, 2018
339dc00
Rename is_local to is_managed
rgrinberg May 14, 2018
0c9fb4a
cleanup
May 15, 2018
3a11ac8
Use source code ordering for Path.compare
May 15, 2018
f9bec51
Remove public uses of Path.kind
rgrinberg May 15, 2018
d12d021
Retain constructors of paths in sexp conversion
rgrinberg May 15, 2018
b84545d
Implement normalization for external paths
rgrinberg May 16, 2018
8eea911
Make Kind.t abstract
rgrinberg May 16, 2018
e98af3f
Normalize paths in kind.of_string
rgrinberg May 16, 2018
872c421
Make normalize raise on invalid path
rgrinberg May 16, 2018
4c1cad2
Add setting workspace root global var
rgrinberg May 16, 2018
ae9e8e9
External.descendant
rgrinberg May 16, 2018
f02026f
Make Kind.of_string translate absolute paths into local paths when po…
rgrinberg May 16, 2018
c0cc86f
Fix bootstrap
rgrinberg May 16, 2018
0830d60
Fix error message
rgrinberg May 16, 2018
1230776
Rename drop_build_dir to as_relative_to_build_dir
rgrinberg May 16, 2018
dd1bf32
Remove trying to convert external paths to build paths
rgrinberg May 16, 2018
18782ab
Make Local.t abstract
rgrinberg May 17, 2018
62de740
Fix reach_for_running
rgrinberg May 20, 2018
dc1342f
Make Path.External.t abstract
rgrinberg May 20, 2018
99cedbc
Remove broken normalization
rgrinberg May 21, 2018
c236947
Make sure sexp conversion functions in path.ml preserve the invariants
May 21, 2018
f44b78d
_
May 21, 2018
0138034
Make extend_basename more efficient
rgrinberg May 21, 2018
b57a2a6
Fix accidental capitalization
rgrinberg May 21, 2018
e587c42
Use Kind.append_local to concat local paths
rgrinberg May 21, 2018
a55b1e5
rework Path.relative/of_string
May 21, 2018
0d01d5c
Add Path.Local.Prefix
May 21, 2018
5e2a6d9
Use Path.pp_debug in path expect tests
rgrinberg May 21, 2018
3397abe
Use lazy values for root and build_dir
May 21, 2018
e76abb6
Intern external paths
rgrinberg May 22, 2018
2e287ec
Re-implement Local functions in terms of is_root
rgrinberg May 22, 2018
f25b9a7
Intern local paths
rgrinberg May 23, 2018
4da825f
Simplify some conditionals into ||
rgrinberg May 23, 2018
54bde97
Remove functions from Path.Local
rgrinberg May 23, 2018
a076300
Do not marshall Path.t
rgrinberg May 23, 2018
c57f6af
Don't marshal Action.t
rgrinberg May 23, 2018
dd2e3d1
Sort paths based on their values before marshalling
rgrinberg May 23, 2018
98e9fb9
Add Path.Set.to_alpha_list
rgrinberg May 24, 2018
df5e0ac
Make path lists passed to user deterministic
rgrinberg May 24, 2018
c76bc54
Restore old printing order
rgrinberg May 24, 2018
07de911
Use greedy resizing for interned paths
rgrinberg May 24, 2018
386ab5e
Fix for generative sets
rgrinberg May 28, 2018
6b724c9
Remove interning
rgrinberg May 28, 2018
1a774e3
Remove sorting from to_alpha_list
rgrinberg May 28, 2018
dbd13da
Fix compare_val issue
rgrinberg May 28, 2018
06d69b7
Represent root as "."
May 28, 2018
e6ad475
Avoid a few extra comparisons
May 28, 2018
7662acf
Fix Local.descendant
May 28, 2018
2426bc3
Slightly faster implem of Path.reach
May 28, 2018
b60ea2e
Get rid of Clflags.workspace_root
May 31, 2018
487f3d4
Restore utop target
May 31, 2018
bc33d09
Remove Path.Set.to_alpha_list
May 31, 2018
49ba8ed
Rename Path.absolute to Path.of_filename_relative_to_initial_cwd
May 31, 2018
a297a52
Merge remote-tracking branch 'origin/master' into sym-path
May 31, 2018
fbcc458
Update tests
May 31, 2018
110e79f
Constraint on the form of the build directory a bit more
May 31, 2018
c2d3e62
Add a test with invalid result
May 31, 2018
8d4a3b9
Merge remote-tracking branch 'origin/master' into sym-path
Jun 1, 2018
01b3b5d
Fix Path.relative when going from the source tree to the build directory
Jun 1, 2018
4d3021f
Add a test with a build directory not starting with _
Jun 1, 2018
8694b22
Fix Path.is_build_dir and add Path.is_strict_descendant_of_build_dir
Jun 1, 2018
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
27 changes: 21 additions & 6 deletions bin/main.ml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type common =
; auto_promote : bool
; force : bool
; ignore_promoted_rules : bool
; build_dir : string
; (* Original arguments for the external-lib-deps hint *)
orig_args : string list
; config : Config.t
Expand All @@ -36,7 +37,7 @@ let set_common c ~targets =
Clflags.capture_outputs := c.capture_outputs;
if c.root <> Filename.current_dir_name then
Sys.chdir c.root;
Clflags.workspace_root := Sys.getcwd ();
Clflags.workspace_root := Path.External.cwd ();
Clflags.diff_command := c.diff_command;
Clflags.auto_promote := c.auto_promote;
Clflags.force := c.force;
Expand All @@ -45,7 +46,9 @@ let set_common c ~targets =
[ ["dune"; "external-lib-deps"; "--missing"]
; c.orig_args
; targets
]
];
Path.set_root !Clflags.workspace_root;
Path.set_build_dir (Path.Kind.of_string c.build_dir)

let restore_cwd_and_execve common prog argv env =
let env = Env.to_unix env in
Expand Down Expand Up @@ -224,7 +227,9 @@ let common =
orig)
x
display
build_dir
=
let build_dir = Option.value ~default:"_build" build_dir in
Copy link

Choose a reason for hiding this comment

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

Why not put the default value in the Arg.(value ...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's because the default would have to be duplciated for bootstrap, but I'm no longer sure. I agree that making the default part of the CLI is correct.

let root, to_cwd =
match root with
| Some dn -> (dn, [])
Expand Down Expand Up @@ -280,6 +285,7 @@ let common =
List.map ~f:Package.Name.of_string (String.split s ~on:',')))
; x
; config
; build_dir
}
in
let docs = copts_sect in
Expand Down Expand Up @@ -518,6 +524,14 @@ let common =
& info ["x"] ~docs
~doc:{|Cross-compile using this toolchain.|})
in
let build_dir =
let doc = "Specified build directory. _build if unspecified" in
Arg.(value
& opt (some string) None
& info ["build-dir"] ~docs ~docv:"FILE"
~env:(Arg.env_var ~doc "DUNE_BUILD_DIR")
~doc)
in
let diff_command =
Arg.(value
& opt (some string) None
Expand All @@ -537,6 +551,7 @@ let common =
$ merged_options
$ x
$ display
$ build_dir
)

let installed_libraries =
Expand Down Expand Up @@ -593,7 +608,7 @@ let resolve_package_install setup pkg =
|> List.map ~f:Package.Name.to_string))

let target_hint (setup : Main.setup) path =
assert (Path.is_local path);
assert (Path.is_managed path);
let sub_dir = Option.value ~default:path (Path.parent path) in
let candidates = Build_system.all_targets setup.build_system in
let candidates =
Expand Down Expand Up @@ -650,7 +665,7 @@ let resolve_targets ~log common (setup : Main.setup) user_targets =
check_path path;
if Path.is_root path then
die "@@ on the command line must be followed by a valid alias name"
else if not (Path.is_local path) then
else if not (Path.is_managed path) then
die "@@ on the command line must be followed by a relative path"
else
Ok [Alias_rec path]
Expand All @@ -660,7 +675,7 @@ let resolve_targets ~log common (setup : Main.setup) user_targets =
let can't_build path =
Error (path, target_hint setup path);
in
if not (Path.is_local path) then
if not (Path.is_managed path) then
Ok [File path]
else if Path.is_in_build_dir path then begin
if Build_system.is_target setup.build_system path then
Expand Down Expand Up @@ -1269,8 +1284,8 @@ let utop =
; `Blocks help_secs
] in
let go common dir ctx_name args =
set_common common ~targets:[dir];
let utop_target = dir |> Path.of_string |> Utop.utop_exe |> Path.to_string in
set_common common ~targets:[utop_target];
let log = Log.create common in
let (build_system, context, utop_path) =
(Main.setup ~log common >>= fun setup ->
Expand Down
17 changes: 5 additions & 12 deletions src/action.ml
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ module Unexpanded = struct
| List _ -> t sexp

let check_mkdir loc path =
if not (Path.is_local path) then
if not (Path.is_managed path) then
Loc.fail loc
"(mkdir ...) is not supported for paths outside of the workspace:\n\
\ %a\n"
Expand Down Expand Up @@ -849,20 +849,13 @@ let rec exec t ~ectx ~dir ~env ~stdout_to ~stderr_to =
Path.rm_rf path;
Fiber.return ()
| Mkdir path ->
(match Path.kind path with
| External _ ->
(* Internally we make sure never to do that, and [Unexpanded.*expand] check that *)
Exn.code_error
"(mkdir ...) is not supported for paths outside of the workspace"
[ "mkdir", Path.sexp_of_t path ]
| Local path ->
Path.Local.mkdir_p path);
Path.mkdir_p path;
Fiber.return ()
| Digest_files paths ->
let s =
let data =
List.map paths ~f:(fun fn ->
(fn, Utils.Cached_digest.file fn))
(Path.to_string fn, Utils.Cached_digest.file fn))
in
Digest.string
(Marshal.to_string data [])
Expand Down Expand Up @@ -936,7 +929,7 @@ let exec ~targets ~context t =
let sandbox t ~sandboxed ~deps ~targets =
Progn
[ Progn (List.filter_map deps ~f:(fun path ->
if Path.is_local path then
if Path.is_managed path then
Some (Ast.Symlink (path, sandboxed path))
else
None))
Expand All @@ -946,7 +939,7 @@ let sandbox t ~sandboxed ~deps ~targets =
~f_path:(fun ~dir:_ p -> sandboxed p)
~f_program:(fun ~dir:_ x -> Result.map x ~f:sandboxed)
; Progn (List.filter_map targets ~f:(fun path ->
if Path.is_local path then
if Path.is_managed path then
Some (Ast.Rename (sandboxed path, path))
else
None))
Expand Down
2 changes: 1 addition & 1 deletion src/build_interpret.ml
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ let static_deps t ~all_targets ~file_tree =
(* diml: we should probably warn in this case as well *)
()
end;
state := G_evaluated (Path.Set.to_list result);
state := G_evaluated (Path.Set.to_alpha_list result);
let action_deps = Path.Set.union result acc.action_deps in
{ acc with action_deps }
end
Expand Down
53 changes: 25 additions & 28 deletions src/build_system.ml
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ type t =
[(deps (filename + contents), targets (filename only), action)] *)
trace : (Path.t, Digest.t) Hashtbl.t
; file_tree : File_tree.t
; mutable local_mkdirs : Path.Local.Set.t
; mutable local_mkdirs : Path.Set.t
; mutable dirs : (Path.t, Dir_status.t) Hashtbl.t
; mutable gen_rules :
(dir:Path.t -> string list -> extra_sub_directories_to_keep) String.Map.t
Expand All @@ -389,7 +389,7 @@ type t =
}

let string_of_paths set =
Path.Set.to_list set
Path.Set.to_alpha_list set
|> List.map ~f:(fun p -> sprintf "- %s"
(Path.to_string_maybe_quoted
(Path.drop_optional_build_context p)))
Expand All @@ -406,7 +406,7 @@ let get_dir_status t ~dir =
else if dir = Path.build_dir then
(* Not allowed to look here *)
Dir_status.Loaded Path.Set.empty
else if not (Path.is_local dir) then
else if not (Path.is_managed dir) then
Dir_status.Loaded
(match Path.readdir dir with
| exception _ -> Path.Set.empty
Expand Down Expand Up @@ -601,24 +601,20 @@ let clear_targets_digests_after_rule_execution targets =

let make_local_dirs t paths =
Path.Set.iter paths ~f:(fun path ->
match Path.kind path with
| Local path ->
if not (Path.Local.Set.mem t.local_mkdirs path) then begin
Path.Local.mkdir_p path;
t.local_mkdirs <- Path.Local.Set.add t.local_mkdirs path
end
| _ -> ())
if Path.is_managed path && not (Path.Set.mem t.local_mkdirs path) then begin
Path.mkdir_p path;
t.local_mkdirs <- Path.Set.add t.local_mkdirs path
end)

let make_local_parent_dirs t paths ~map_path =
Path.Set.iter paths ~f:(fun path ->
match Path.kind (map_path path) with
| Local path when not (Path.Local.is_root path) ->
let parent = Path.Local.parent path in
if not (Path.Local.Set.mem t.local_mkdirs parent) then begin
Path.Local.mkdir_p parent;
t.local_mkdirs <- Path.Local.Set.add t.local_mkdirs parent
end
| _ -> ())
let path = map_path path in
if Path.is_managed path then (
Option.iter (Path.parent path) ~f:(fun parent ->
if not (Path.Set.mem t.local_mkdirs parent) then begin
Path.mkdir_p parent;
t.local_mkdirs <- Path.Set.add t.local_mkdirs parent
end)))

let sandbox_dir = Path.relative Path.build_dir ".sandbox"

Expand Down Expand Up @@ -713,15 +709,16 @@ let rec compile_rule t ?(copy_source=false) pre_rule =
>>= fun (action, dyn_deps) ->
make_local_parent_dirs t targets ~map_path:(fun x -> x);
let all_deps = Path.Set.union static_deps dyn_deps in
let all_deps_as_list = Path.Set.to_list all_deps in
let targets_as_list = Path.Set.to_list targets in
let all_deps_as_list = Path.Set.to_alpha_list all_deps in
let targets_as_list = Path.Set.to_alpha_list targets in
let hash =
let trace =
(List.map all_deps_as_list ~f:(fun fn ->
(fn, Utils.Cached_digest.file fn)),
targets_as_list,
Option.map context ~f:(fun c -> c.name),
action)
( all_deps_as_list
|> List.map ~f:(fun fn ->
(Path.to_string fn, Utils.Cached_digest.file fn)),
List.map targets_as_list ~f:Path.to_string,
Option.map context ~f:(fun c -> c.name),
Action.for_shell action)
in
Digest.string (Marshal.to_string trace [])
in
Expand Down Expand Up @@ -760,7 +757,7 @@ let rec compile_rule t ?(copy_source=false) pre_rule =
| Some sandbox_dir ->
Path.rm_rf sandbox_dir;
let sandboxed path =
if Path.is_local path then
if Path.is_managed path then
Path.append sandbox_dir path
else
path
Expand Down Expand Up @@ -1191,7 +1188,7 @@ let create ~contexts ~file_tree ~hook =
; files = Hashtbl.create 1024
; packages = Hashtbl.create 1024
; trace = Trace.load ()
; local_mkdirs = Path.Local.Set.empty
; local_mkdirs = Path.Set.empty
; dirs = Hashtbl.create 1024
; load_dir_stack = []
; file_tree
Expand Down Expand Up @@ -1483,7 +1480,7 @@ let get_collector t ~dir =
"Build_system.get_collector called on source directory"
else if dir = Path.build_dir then
"Build_system.get_collector called on build_dir"
else if not (Path.is_local dir) then
else if not (Path.is_managed dir) then
"Build_system.get_collector called on external directory"
else
"Build_system.get_collector called on closed directory")
Expand Down
4 changes: 3 additions & 1 deletion src/clflags.ml
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
open Stdune

(*let ocaml_comp_flags = ref ["-g"]*)
let g = ref true
let debug_findlib = ref false
let warnings = ref "-40"
let debug_dep_path = ref false
let workspace_root = ref "."
let workspace_root = ref Path.External.initial_cwd
let external_lib_deps_hint = ref []
let capture_outputs = ref true
let debug_backtraces = ref false
Expand Down
4 changes: 3 additions & 1 deletion src/clflags.mli
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
open Stdune

(** Command line flags *)

(** Compilation flags for OCaml files *)
Expand All @@ -16,7 +18,7 @@ val debug_findlib : bool ref
val warnings : string ref

(** The path to the workspace root *)
val workspace_root : string ref
val workspace_root : Path.External.t ref

(** The command line for "Hint: try: jbuilder external-lib-deps ..." *)
val external_lib_deps_hint : string list ref
Expand Down
4 changes: 2 additions & 2 deletions src/config.ml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ let local_install_lib_dir ~context ~package =
(Path.relative (local_install_dir ~context) "lib")
package

let dev_null = Path.of_string (if Sys.win32 then "nul" else "/dev/null")
let dev_null = Path.absolute (if Sys.win32 then "nul" else "/dev/null")

let jbuilder_keep_fname = ".jbuilder-keep"

Expand Down Expand Up @@ -108,7 +108,7 @@ let t =
})

let user_config_file =
Path.relative (Path.of_string Xdg.config_dir) "dune/config"
Path.relative (Path.absolute Xdg.config_dir) "dune/config"

let load_config_file p =
t (Io.Sexp.load_many_as_one p)
Expand Down
14 changes: 7 additions & 7 deletions src/configurator/v1.ml
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ let run t ~dir cmd =
(Filename.quote stdout_fn)
(Filename.quote stderr_fn)
in
let stdout = Io.read_file (Path.of_string stdout_fn) in
let stderr = Io.read_file (Path.of_string stderr_fn) in
let stdout = Io.read_file (Path.absolute stdout_fn) in
let stderr = Io.read_file (Path.absolute stderr_fn) in
logf t "-> process exited with code %d" exit_code;
logf t "-> stdout:";
List.iter (String.split_lines stdout) ~f:(logf t " | %s");
Expand Down Expand Up @@ -237,7 +237,7 @@ let compile_and_link_c_prog t ?(c_flags=[]) ?(link_flags=[]) code =
let c_fname = base ^ ".c" in
let obj_fname = base ^ t.ext_obj in
let exe_fname = base ^ ".exe" in
Io.write_file (Path.of_string c_fname) code;
Io.write_file (Path.absolute c_fname) code;
logf t "compiling c program:";
List.iter (String.split_lines code) ~f:(logf t " | %s");
let run_ok args =
Expand Down Expand Up @@ -267,7 +267,7 @@ let compile_c_prog t ?(c_flags=[]) code =
let base = dir ^/ "test" in
let c_fname = base ^ ".c" in
let obj_fname = base ^ t.ext_obj in
Io.write_file (Path.of_string c_fname) code;
Io.write_file (Path.absolute c_fname) code;
logf t "compiling c program:";
List.iter (String.split_lines code) ~f:(logf t " | %s");
let run_ok args =
Expand All @@ -284,7 +284,7 @@ let compile_c_prog t ?(c_flags=[]) code =
]
])
in
if ok then Ok (Path.of_string obj_fname) else Error ()
if ok then Ok (Path.absolute obj_fname) else Error ()

let c_test t ?c_flags ?link_flags code =
match compile_and_link_c_prog t ?c_flags ?link_flags code with
Expand Down Expand Up @@ -413,7 +413,7 @@ const char *s%i = "BEGIN-%i-false-END";
logf t "writing header file %s" fname;
List.iter lines ~f:(logf t " | %s");
let tmp_fname = fname ^ ".tmp" in
Io.write_lines (Path.of_string tmp_fname) lines;
Io.write_lines (Path.absolute tmp_fname) lines;
Sys.rename tmp_fname fname
end

Expand Down Expand Up @@ -479,7 +479,7 @@ module Pkg_config = struct
end

let write_flags fname s =
let path = Path.of_string fname in
let path = Path.in_source fname in
let sexp = Usexp.List(List.map ~f:Usexp.atom_or_quoted_string s) in
Io.write_file path (Usexp.to_string sexp)

Expand Down
4 changes: 2 additions & 2 deletions src/dune_project.ml
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ end = struct
None

let anonymous path =
if Path.is_local path then
if Path.is_managed path then
Some (Anonymous path)
else
None
Expand Down Expand Up @@ -107,7 +107,7 @@ end = struct
|> List.tl
|> String.concat ~sep:"/")
in
if not (Path.is_local p) then invalid s;
if not (Path.is_managed p) then invalid s;
Copy link
Member Author

Choose a reason for hiding this comment

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

@diml I think this check and the one above can perhaps be tightened a little. E.g. Path.is_in_source_tree instead.

Copy link

Choose a reason for hiding this comment

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

Indeed

Anonymous p
| _ when validate s -> Named s
| _ -> invalid s
Expand Down
Loading