Fix workspace lookup in conjunction with --root #997

rgrinberg · 2018-07-10T22:01:20Z

The workspace is specified to the initial CWD hence we must convert it a path
relative to it

Not sure if we should include this in 1.0 or not. But if this doesn't make it to 1.0 then it will need a CHANGELOG entry.

ghost · 2018-07-11T07:43:18Z

test/blackbox-tests/test-cases/workspaces/run.t

@@ -25,7 +25,8 @@ analogously, jbuilder will ignore it
 specifying the workspace file is possible:

  $ dune build --root custom-workspace --workspace custom-workspace/dune-workspace.dev
-  Error: workspace file custom-workspace/dune-workspace.dev does not exist
+  File "/Users/rgrinberg/reps/dune/_build/default/test/blackbox-tests/test-cases/workspaces/custom-workspace/dune-workspace.dev", line 2, characters 10-24:


This contains an absolute path

ghost · 2018-07-11T07:44:57Z

Do we have the same issue for the --config-file option? What about overriding the Cmdliner.Arg.file value and making it return a Path.t?

rgrinberg · 2018-07-11T08:13:50Z

Ah yeah. This would be an issue for any of these file arguments.

ghost · 2018-07-11T08:27:24Z

BTW, this is not a big breaking change. The file combinator of cmdliner fails if the file doesn't exist. So the only case where this PR would break something is the following one: dune build --root foo --workspace blah where both blah and foo/blah exist. This seems very unlikely, so we can just do the change and release it as part of the next release.

rgrinberg · 2018-07-11T08:39:37Z

One slightly annoying thing about this is that we still need to save the original string for dumping orig_args accurately. Which is why I didn't create a custom type earlier. I'm wondering, isn't there a better way to dump these original args? Like by looking into Sys.argv perhaps?

rgrinberg · 2018-07-11T08:55:19Z

@diml you can see the last commit where I did this. orig_args isn't being faithfully reproduced anymore.

ghost · 2018-07-11T10:21:23Z

bin/main.ml

@@ -189,6 +188,11 @@ let find_root () =
 let package_name =
  Arg.conv ((fun p -> Ok (Package.Name.of_string p)), Package.Name.pp)

+let path_arg =


What about:

module Arg = struct include Arg let file = ... end

?

Just to make sure we don't use the wrong combinator.

Sure, that makes sense. And the original args issue?

ghost · 2018-07-11T10:40:23Z

I suppose the type can be string * Path.t rather than Path.t. Initially we were using Sys.argv but it caused problems so we changed it. I can't remember what the issue was though.

rgrinberg · 2018-07-11T10:56:53Z

I suppose the type can be string * Path.t rather than Path.t. Initially we were using Sys.argv but it caused problems so we changed it. I can't remember what the issue was though.

Should we try and bring it back? The current orig_args is quite janky anyway. For example, it will not recall the original --build-dir in the execution.

ghost · 2018-07-11T11:34:17Z

I suppose we can. The current method is painful and I believe there are regression test for the original issues.

The workspace is specified to the initial CWD hence we must convert it a path relative to it Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

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

This path argument will take paths relatively to the initial CWD Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

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

rgrinberg · 2018-07-11T14:49:07Z

Hmm, yeah the original args are no good because you have to parse out the subcommand out of them. Not very pleasant. I will do your approach and just maintain the original string as well.

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

rgrinberg · 2018-07-11T14:57:58Z

@diml ready for another review.

ghost · 2018-07-11T15:06:29Z

bin/main.ml

+    let conv = Arg.conv ((fun p -> Ok p), Format.pp_print_string)
+  end
+
+  let path = Path.conv


Can we have let file = path as well? Just to make sure we don't use the wrong thing

ghost · 2018-07-11T15:07:22Z

Hmm, yeah the original args are no good because you have to parse out the subcommand out of them.

So cmdliner requires that the subcommand is the first argument, so it might actually be ok. I think I didn't know that at the time

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

rgrinberg · 2018-07-11T15:32:35Z

So cmdliner requires that the subcommand is the first argument, so it might actually be ok. I think I didn't know that at the time

Hmm, yeah. Might be worth trying another time. With things like exec, we also need to remove everything after and including --. Also, we need to make sure to get rid of options that are specific to build/exec (if those exist).

@rgrinberg

CHANGES: - Fix lookup of command line specified files when `--root` is given. Previously, passing in `--root` in conjunction with `--workspace` or `--config` would not work correctly (ocaml/dune#997, @rgrinberg) - Add support for customizing env nodes in workspace files. The `env` stanza is now allowed in toplevel position in the workspace file, or for individual contexts. This feature requires `(dune lang 1.1)` (ocaml/dune#1038, @rgrinberg) - Add `enabled_if` field for aliases and tests. This field controls whether the test will be ran using a boolean expression language. (ocaml/dune#819, @rgrinberg) - Make `name`, `names` fields optional when a `public_name`, `public_names` field is provided. (ocaml/dune#1041, fix ocaml/dune#1000, @rgrinberg) - Interpret `X` in `--libdir X` as relative to `PREFIX` when `X` is relative (ocaml/dune#1072, fix ocaml/dune#1070, @diml) - Add support for multi directory libraries by writing `(include_subdirs unqualified)` (ocaml/dune#1034, @diml) - Add `(staged_pps ...)` to support staged ppx rewriters such as ones using the OCaml typer like `ppx_import` (ocaml/dune#1080, fix ocaml/dune#193, @diml) - Use `-opaque` in the `dev` profile. This option trades off binary quality for compilation speed when compiling .cmx files. (ocaml/dune#1079, fix ocaml/dune#1058, @rgrinberg) - Fix placeholders in `dune subst` documentation (ocaml/dune#1090, @emillon, thanks @trefis for the bug report) - Add locations to errors when a missing binary in PATH comes from a dune file (ocaml/dune#1096, fixes ocaml/dune#1095, @rgrinberg)

rgrinberg requested review from a user and emillon July 10, 2018 22:01

ghost suggested changes Jul 11, 2018

View reviewed changes

emillon changed the title ~~Fix workspace lookup in cojnuction with --root~~ Fix workspace lookup in conjunction with --root Jul 11, 2018

rgrinberg added 5 commits July 11, 2018 16:41

Fix workspace lookup in cojnuction with --root

ab6bb0d

The workspace is specified to the initial CWD hence we must convert it a path relative to it Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Fix the workspace test not to leak absolute paths

93fb319

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

Use path argument for workspace and config file

02beadb

This path argument will take paths relatively to the initial CWD Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Move custom args to Arg submodule

ab85720

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

Update CHANGELOG

630c10e

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

Improve the path custom conv to use original path when restoring args

f9c1d56

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

rgrinberg force-pushed the fix-workspace-lookup branch from 6e352ae to f9c1d56 Compare July 11, 2018 14:57

ghost suggested changes Jul 11, 2018

View reviewed changes

Add alias just to be sure

b4b7dde

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

ghost approved these changes Jul 11, 2018

View reviewed changes

rgrinberg merged commit 5a31b93 into ocaml:master Jul 11, 2018

rgrinberg deleted the fix-workspace-lookup branch July 11, 2018 16:31

rgrinberg mentioned this pull request Aug 6, 2018

[new release] dune (1.1.0) ocaml/opam-repository#12442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix workspace lookup in conjunction with --root #997

Fix workspace lookup in conjunction with --root #997

rgrinberg commented Jul 10, 2018

ghost Jul 11, 2018

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

ghost Jul 11, 2018

rgrinberg Jul 11, 2018

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

ghost Jul 11, 2018

rgrinberg Jul 11, 2018

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

Fix workspace lookup in conjunction with --root #997

Fix workspace lookup in conjunction with --root #997

Conversation

rgrinberg commented Jul 10, 2018

ghost Jul 11, 2018

Choose a reason for hiding this comment

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

ghost Jul 11, 2018

Choose a reason for hiding this comment

rgrinberg Jul 11, 2018

Choose a reason for hiding this comment

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

rgrinberg commented Jul 11, 2018

ghost Jul 11, 2018

Choose a reason for hiding this comment

rgrinberg Jul 11, 2018

Choose a reason for hiding this comment

ghost commented Jul 11, 2018

rgrinberg commented Jul 11, 2018