Symbolic Paths #744

rgrinberg · 2018-05-04T10:04:55Z

This is my attempt at porting to symbolic paths. Putting this up while this is WIP as I'm not sure I'm going in the right direction as I think you imagined this change being simpler than it's turning out for me here.

Repeating the question from slack: I'm not sure if Path.parent should escape the build dir for paths inside the build dir.

ghost · 2018-05-04T15:40:38Z

That seems fine to me, I expected the implementation of Path to change quite a bit. I think most of the complexity comes from the fact that paths inside the build directory are no longer always local and this needs to be handled correctly.

BTW, looking at how Path.kind and Path.is_local are used outside of Path, it seems that we often have two cases:

either a path is an external path
either a path is in the source tree or build directory

Before, the second class corresponded exactly to local paths, but this is no longer the case now. I think we should call this class managed paths and update the code accordingly, for instance with a Path.is_managed function.

ghost · 2018-05-04T15:43:39Z

Repeating the question from slack: I'm not sure if Path.parent should escape the build dir for paths inside the build dir.

It shouldn't. If we rely on Path.parent Path.build_dir being Path.root in some part of the code, we need to update the code to stop at Path.build_dir.

Note that this can potentially break user jbuild files if they try to escape the build directory to access the source tree directly.

rgrinberg · 2018-05-07T10:04:22Z

Before, the second class corresponded exactly to local paths, but this is no longer the case now. I think we should call this class managed paths and update the code accordingly, for instance with a Path.is_managed function.

I trid this change out locally and I think that it effectively obsoletes Path.is_local. I don't think there are any instances that rely on the path actually being external. Though I think there might be some cases where we can be a little more strict and explicitly check for in_source_dir vs. in_build_dir.

rgrinberg · 2018-05-07T10:08:10Z

Note that this can potentially break user jbuild files if they try to escape the build directory to access the source tree directly.

Yeah, but I think that's fine. I guess we should really be preventing the user from doing this by making sure the actions don't act on source dirs.

rgrinberg · 2018-05-07T10:42:45Z

It shouldn't. If we rely on Path.parent Path.build_dir being Path.root in some part of the code, we need to update the code to stop at Path.build_dir.

OK. How about we change Path.parent to be t -> t option to have a value for when we can't advance past root (with a Path.parent_exn for situations where we know this is allowed, perhaps).

ghost · 2018-05-08T10:35:47Z

OK. How about we change Path.parent to be t -> t option to have a value for when we can't advance past root (with a Path.parent_exn for situations where we know this is allowed, perhaps).

That seems fine

rgrinberg · 2018-05-09T05:24:30Z

Okay, so this is getting closer to completion. There's a couple of other points that I think we should handle:

Recognize external paths that point to the build directory in Path.of_string.
Change Path.relative root "_build/foobar". Currently, this will correctly change the path to a build directory but I suppose that this should only work when the build dir is under the source tree.

Back to the point with managed vs. local. Do you think we should go ahead with this change? Internally in Path we already have a Kind module that also uses a Local constructor so it's a bit confusing that Path.is_local and Kind.is_local will not agree.

ghost · 2018-05-09T06:14:37Z

Yh, I think we need to make this change

rgrinberg · 2018-05-12T13:54:50Z

Ok, @diml this is getting close. Now the build dir is allowed to be external vs. local so it should be easy to just set it to the other thing. There are still some issues with other functions (see path.mlt), but I'm planning to fix those.

rgrinberg · 2018-05-12T13:59:44Z

src/stdune/path.ml

+    if String.is_prefix s ~prefix:"_build" then (
+      Exn.code_error "in_source_tree: path is in build dir"
+        [ "s", Sexp.To_sexp.string s ]
+    );


I'll get rid of these checks, they were just useful for debugging.

rgrinberg · 2018-05-14T03:43:22Z

@diml latest is issue is with regards to configurator:

Configurator also uses path functions now, so it should set the build dir before doing so. I'm thinking that we should simply pass the build dir via DUNE_CONFIGURATOR. But to do that, we should really have a way of packing multiple values into that env var via sexps. Can we get the Sexp parsing stuff moved to Usexp yet?

The above isn't actually necessary.

rgrinberg · 2018-05-14T13:15:35Z

Okay so --build-dir seems to finally work so reviewing can start. Sorry for the mess, I'll tidy up things whenever they don't make sense.

The failure on 4.02.3 is reproducible, but I don't yet understand what's causing it and why bootstrap works on other versions. Any ideas here would be welcome.

ghost · 2018-05-14T13:19:54Z

Maybe it's due to some change in the Unix library?

rgrinberg · 2018-05-14T17:01:33Z

Maybe it's due to some change in the Unix library?

It was just a bug on my part. The only reason it was only reproducible on 4.02.3 was because that's where we used sandboxes.

ghost · 2018-05-15T13:36:37Z

src/stdune/path.ml

+let drop_build_dir p =
+  match build_dir_kind (), Kind.of_string p with
+  | Kind.External bd, Kind.External p ->
+    let open Option.O in


This seems a bit fragile to me. Because we don't normalize external paths, we might miss cases here. Why do we need this exactly?

If the user does something like $ jbuilder build --build-dir /foo/bar /foo/bar/baz I'd expect the path to be resolved to a build dir.

I also think that the db file also relies on this. Though we can fix that by changing the sexp reading/writing not to erase the the constructor of Path.t.

Yes, but what if the the user does jbuilder build --build-dir /foo//bar /foo/bar/baz?

ghost · 2018-05-15T13:50:20Z

The last two uses of Path.kind in the code base looks incorrect to me. In the two cases, we now want to match on wether the path is managed or not rather than local or external.

ghost · 2018-05-15T13:54:51Z

I think it would be easier to reason about the code if we have the guarantee that exactly one of the following assertion holds:

either the build directory is an absolute path that doesn't point inside the source tree
either it is relative to the source tree root

If the user passes an absolute path for the build directory, we don't check whether it points to the source tree or not.

I suggest to do the following: we implement a function similar to realpath to normalize absolute paths. If the build directory is given as an absolute path, then we pass it through realpath, we pass the workspace root through realpath as well and we compare them. If the build directory is a descendant of the root, then we convert it to a local path.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

This avoids a few conversions Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

ghost · 2018-05-28T17:21:59Z

src/stdune/path.ml

+    with
+    | Unix.Unix_error (EEXIST, _, _) -> ()
+    | Unix.Unix_error (ENOENT, _, _) ->
+      Exn.fatalf "Cannot create external build directory %s. \


We have External.mkdir_p now, should we use it here and in Path.mkdir_p?

It's not quite the same b/c mkdir_p would re-create the parents. What should this do when foo doesn't exist for example:

$ jbuilder build --build-dir /home/ocaml/foo/bar

I would want foo to be created.

ghost · 2018-05-28T17:34:51Z

src/stdune/path.ml

+  (build_dir, build_dir_prefix, set_build_dir)
+
+module T : sig
+  type t = private


Why the private?

rgrinberg · 2018-05-28T17:41:52Z

I used the constructor functions to help with debugging. I think I removed those extra checks since, so we can get rid of it now.

…

On Tue, May 29, 2018, 12:40 AM Jérémie Dimino ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/stdune/path.ml <#744 (comment)>: > @@ -490,29 +776,39 @@ let unlink_no_err t = try unlink t with _ -> () let build_dir_exists () = is_directory build_dir -let ensure_build_dir_exists () = Local.mkdir_p build_dir - -let extend_basename t ~suffix = t ^ suffix +let ensure_build_dir_exists () = + match kind build_dir with + | Local p -> Local.mkdir_p p + | External p -> + let p = External.to_string p in + try + Unix.mkdir p 0o777 + with + | Unix.Unix_error (EEXIST, _, _) -> () + | Unix.Unix_error (ENOENT, _, _) -> + Exn.fatalf "Cannot create external build directory %s. \ We have External.mkdir_p now, should we use it here and in Path.mkdir_p? ------------------------------ In src/stdune/path.ml <#744 (comment)>: > + let build_dir = lazy ( + match !build_dir with + | None -> + Exn.code_error "build_dir: cannot use build dir before it's set" [] + | Some build_dir -> build_dir) + in + let build_dir_prefix = lazy ( + match !build_dir_prefix with + | None -> + Exn.code_error "build_dir: cannot use build dir before it's set" [] + | Some prefix -> prefix) + in + (build_dir, build_dir_prefix, set_build_dir) + +module T : sig + type t = private Why the private? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#744 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-1VrY2MJ-SUzG0goa0Zlh49AoIcKks5t3DZzgaJpZM4TyZwc> .

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

ghost · 2018-05-31T17:15:10Z

bin/main.ml

    =
+    let build_dir = Option.value ~default:"_build" build_dir in


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

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.

I find this confusing. If we re-enable interning, we'll need to make sure the ordering in sets preserve the natural ordering. Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

The previous name was not very clear Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

rgrinberg · 2018-05-31T20:08:33Z

test/unit-tests/path.mlt

+|}]
+
+(* This is not right, but kind of annoying to fix :/ *)
+Path.relative (r "foo") "../_build"


Previously, I tried addressing this by having logic in the in_source_tree smart constructor to automatically change such paths to In_build_dir. This wasn't really necessary to make the feature work, so I dropped it. In any case, I wasn't comfortable with that approach either.

rgrinberg · 2018-05-31T20:14:18Z

@diml I've read over your changes and I welcome them all. Github doesn't let me approve since I created this PR. I approve "manually".

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

ghost · 2018-06-01T10:02:07Z

Ok, I pushed a couple of additional changes. I let you review them

rgrinberg · 2018-06-01T10:11:37Z

Okay, looks fine.

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

rgrinberg · 2018-06-04T14:12:34Z

This is in master.

rgrinberg requested a review from a user May 4, 2018 10:04

rgrinberg mentioned this pull request May 7, 2018

Alias0.dir is always in build_dir #746

Merged

rgrinberg force-pushed the sym-path branch from 05ce140 to bbe3542 Compare May 8, 2018 17:14

rgrinberg force-pushed the sym-path branch 6 times, most recently from 43b76ee to 7a6921a Compare May 12, 2018 13:52

rgrinberg force-pushed the sym-path branch from 7a6921a to b789ca6 Compare May 12, 2018 13:59

rgrinberg commented May 12, 2018

View reviewed changes

rgrinberg force-pushed the sym-path branch from b789ca6 to 6febd10 Compare May 13, 2018 02:25

rgrinberg mentioned this pull request May 14, 2018

Path.is_in_build_dir test #751

Merged

rgrinberg changed the title ~~Symbolic Paths [WIP]~~ Symbolic Paths May 14, 2018

ghost reviewed May 15, 2018

View reviewed changes

rgrinberg and others added 4 commits May 28, 2018 21:18

Fix compare_val issue

dbd13da

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Represent root as "."

06d69b7

This avoids a few conversions Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Avoid a few extra comparisons

e6ad475

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Fix Local.descendant

7662acf

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

ghost reviewed May 28, 2018

View reviewed changes

Jeremie Dimino added 3 commits May 28, 2018 19:27

Slightly faster implem of Path.reach

2426bc3

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Get rid of Clflags.workspace_root

b60ea2e

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Restore utop target

487f3d4

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

ghost reviewed May 31, 2018

View reviewed changes

Jeremie Dimino added 6 commits May 31, 2018 18:17

Remove Path.Set.to_alpha_list

bc33d09

I find this confusing. If we re-enable interning, we'll need to make sure the ordering in sets preserve the natural ordering. Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Rename Path.absolute to Path.of_filename_relative_to_initial_cwd

49ba8ed

The previous name was not very clear Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Merge remote-tracking branch 'origin/master' into sym-path

a297a52

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Update tests

fbcc458

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Constraint on the form of the build directory a bit more

110e79f

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Add a test with invalid result

c2d3e62

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

ghost force-pushed the sym-path branch from ff76503 to c2d3e62 Compare May 31, 2018 18:10

rgrinberg commented May 31, 2018

View reviewed changes

Jeremie Dimino added 2 commits June 1, 2018 10:01

Merge remote-tracking branch 'origin/master' into sym-path

8d4a3b9

Fix Path.relative when going from the source tree to the build directory

01b3b5d

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

ghost added this to the 1.0.0 milestone Jun 2, 2018

ghost assigned rgrinberg Jun 2, 2018

Jeremie Dimino added 2 commits June 2, 2018 17:58

Add a test with a build directory not starting with _

4d3021f

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Fix Path.is_build_dir and add Path.is_strict_descendant_of_build_dir

8694b22

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

rgrinberg force-pushed the sym-path branch from 9b0c72a to 8694b22 Compare June 2, 2018 10:58

rgrinberg closed this Jun 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Symbolic Paths #744

Symbolic Paths #744

rgrinberg commented May 4, 2018

ghost commented May 4, 2018

ghost commented May 4, 2018

rgrinberg commented May 7, 2018

rgrinberg commented May 7, 2018

rgrinberg commented May 7, 2018

ghost commented May 8, 2018

rgrinberg commented May 9, 2018

ghost commented May 9, 2018

rgrinberg commented May 12, 2018

rgrinberg May 12, 2018

rgrinberg commented May 14, 2018 •

edited

Loading

rgrinberg commented May 14, 2018

ghost commented May 14, 2018

rgrinberg commented May 14, 2018

ghost May 15, 2018

rgrinberg May 15, 2018

ghost May 15, 2018

ghost commented May 15, 2018

ghost commented May 15, 2018

ghost May 28, 2018

rgrinberg May 28, 2018

ghost May 31, 2018

ghost May 28, 2018

rgrinberg commented May 28, 2018 via email •

edited

Loading

ghost May 31, 2018

rgrinberg May 31, 2018

rgrinberg May 31, 2018

rgrinberg commented May 31, 2018

ghost commented Jun 1, 2018

rgrinberg commented Jun 1, 2018

rgrinberg commented Jun 4, 2018

		=
		let build_dir = Option.value ~default:"_build" build_dir in

Symbolic Paths #744

Symbolic Paths #744

Conversation

rgrinberg commented May 4, 2018

ghost commented May 4, 2018

ghost commented May 4, 2018

rgrinberg commented May 7, 2018

rgrinberg commented May 7, 2018

rgrinberg commented May 7, 2018

ghost commented May 8, 2018

rgrinberg commented May 9, 2018

ghost commented May 9, 2018

rgrinberg commented May 12, 2018

Choose a reason for hiding this comment

rgrinberg commented May 14, 2018 • edited Loading

rgrinberg commented May 14, 2018

ghost commented May 14, 2018

rgrinberg commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 15, 2018

ghost commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented May 28, 2018 via email • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented May 31, 2018

ghost commented Jun 1, 2018

rgrinberg commented Jun 1, 2018

rgrinberg commented Jun 4, 2018

rgrinberg commented May 14, 2018 •

edited

Loading

rgrinberg commented May 28, 2018 via email •

edited

Loading