Add a default alias #926

ghost · 2018-06-28T17:38:24Z

This PR adds a special default alias which become the new default target.

Definition

When the default alias is not explictely defined in a directory, it is implicitely defined as follow:

(alias
 (name default)
 (deps (alias_rec install)))

Otherwise the user definition is used.

Default target

The default target is changed from building recursively the alias install to building the default only in the current directory. This allows to redefine what dune build in different directories.

In order to represent that, a new syntax is added to the command line: @@x means building alias x in the current directory. In the end dune build is equivalent to dune build @@default.

dune build -p x still means dune build -p x @install. This is done by adding a command line option --default-target and making -p imply --default-target @install.

Future work

It'd be nice to allow to define what the default target means in any directory of a project. A nice way to do that would be to allow a copy_in_subdirs field to alias stanza that would cause the the alias stanza to be copied in all directories of the project, so that a user may write in the dune-project file:

(alias
 ((name default)
  (copy_in_subdirs)
  (deps (alias_rec install) (alias_rec runtest))))

Implementation details

(alias_rec ...) requires recursive throught the file tree. Since it is now uses for every single directory through the default alias, a bit of laziness was added to Build.t.

ghost · 2018-06-28T17:46:07Z

This behavior is only enable for dune BTW. The behavior for jbuilder is unchanged.

emillon · 2018-06-29T07:25:58Z

CHANGES.md

@@ -93,6 +93,13 @@ next

 - Make `dev` the default build profile (#920, @diml)

+- Add the ability to build an alias non-recursively from the command
+  line by writing `@@alias` (#926, @diml)


About the syntax: I wonder if instead (or in addition to) this @@alias syntax, we could use a directory-based syntax like @./alias. Maybe that could be extended to other directories to support #771?

You can already write a directory after @ (and @@ in this PR). For instance @foo/runtest means alias runtest recursively from foo and @@foo/runtest means alias runtest only in foo.

emillon · 2018-06-29T07:27:00Z

bin/main.ml

+  match Path.extract_build_context dir with
+  | None -> (contexts, dir, name)
+  | Some ("install", _) ->
+    die "Invalid alias: %s.\n\


Would it be useful to pass a loc to have a nicer error message? (or return a result from here)

This is from the command line, so we don't have a loc for it.

emillon · 2018-06-29T07:31:26Z

src/build_interpret.ml

+let no_targets_allowed () =
+  Exn.code_error "No targets allowed under a [Build.lazy_no_targets] \
+                  or [Build.if_file_exists]" []
+[@@inline never]


Out of curiosity, what is the purpose of that annotation? is it just an optimization or is it required?

It's just an optimization, so that the error case code is not inlined in the main function. It's kind of a cold annotation.

emillon · 2018-06-29T07:32:23Z

src/build_system.ml

+  let find_dir_specified_on_command_line ~dir ~file_tree =
+    match File_tree.find_dir file_tree dir with
+    | None ->
+      die "From the command line:\n\


Same remark about die - can we inject a loc here?

This is from the command line as well, so we don't have a more precise location either

emillon · 2018-06-29T09:04:24Z

Was it possible to define an alias name with a @ in it?

rgrinberg · 2018-06-29T10:26:01Z

src/build_system.ml

  let is_standard = function
-    | "runtest" | "install" | "doc" | "doc-private" | "lint" -> true
+    | "runtest" | "install" | "doc" | "doc-private" | "lint" | "default" -> true


is this a breaking change for jbuild projects? IIRC, some users defined their own default aliases. In any case, I'm not sure this will make a big difference.

Not if you use the jbuilder binary, but possibly if you use the dune one. Which seems acceptable to me.

rgrinberg · 2018-06-29T10:30:32Z

src/build_system.ml

-          name (Path.to_string_maybe_quoted src_dir)
-
-  let default     = make "DEFAULT"
+           @{<error>Error@}: Alias %s is empty.\n\


To print things consistently, let' use %S for the alias name.

ghost · 2018-06-29T12:01:00Z

Was it possible to define an alias name with a @ in it?

@emillon yes. It is still possible to refer to it with: @./@toto. We should probably allow the @@ syntax only for dune .

rgrinberg

Looks good. I'm a bit curious as to how much benefit that optimization brings us. IIRC, you've asked me to try something similar before and I remember not really gaining much. But I think this construct might be applicable elsewhere as well.

rgrinberg · 2018-06-30T01:50:51Z

src/build_system.ml

+        fail fn
+      else
+        let fn = Path.append (Path.relative Path.build_dir ctx) fn' in
+        die "Trying to build alias %s but build context %s doesn't exist.%s"


Is this case possible? I'm actually not sure if we can exercise it.

It can happen if you request an alias in _build explicitely from the command line.

rgrinberg · 2018-06-30T01:56:31Z

Oh, and please accept the corrections before merging.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

- if "default" is specified by the user explicitely, use this definition - otherwise assume the following definition: (alias (name default) (deps (alias_rec install))) Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

This is useful for (alias_rec ...) since at definition site we recurse through all sub-directories. This is especially relevant now that we have the default alias which defaults to (alias_rec install). Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

ghost · 2018-07-01T20:50:40Z

Maybe this optimization doesn't buy us that much. I just got into the habbit of doing it... Ideally the compiler would do this optimization itself

rgrinberg · 2018-07-02T05:48:48Z

Jérémie Dimino <notifications@github.com> writes:

+ | Alias (ctx, fn') -> + if String.Map.mem t.contexts ctx then + fail fn + else + let fn = Path.append (Path.relative Path.build_dir ctx) fn' in + die "Trying to build alias %s but build context %s doesn't exist.%s" It can happen if you request an alias in _build explicitely from the command line.

I actually gave this a try, and there's earlier validation that seems to catch this. Perhaps a test case would be helpful here? I don't think this is so important, but it's nice not to add dead code. Perhaps we can even remove earlier validation and just keep this one.

ghost · 2018-07-02T07:08:21Z

There is a validation on the command line. We should be able to hit this case by writing something like this in a toplevel dune file:

  (deps (alias ../invalid-build-context/foo))

ghost requested review from rgrinberg and emillon June 28, 2018 17:39

ghost mentioned this pull request Jun 28, 2018

Support excluding some targets from being built #917

Closed

ghost added this to the 1.0.0 milestone Jun 28, 2018

emillon reviewed Jun 29, 2018

View reviewed changes

rgrinberg reviewed Jun 29, 2018

View reviewed changes

rgrinberg approved these changes Jun 30, 2018

View reviewed changes

jeremiedimino added 4 commits July 1, 2018 21:48

Support @@alias to build an alias non-recursively

21b63d0

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Improve error message for undefined aliases

27602e7

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Add a "default" alias defined as follow:

dbaab9b

- if "default" is specified by the user explicitely, use this definition - otherwise assume the following definition: (alias (name default) (deps (alias_rec install))) Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

ghost merged commit fc9f335 into ocaml:master Jul 1, 2018

ghost mentioned this pull request Jul 10, 2018

[new release] dune (1.0.0) ocaml/opam-repository#12323

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a default alias #926

Add a default alias #926

ghost commented Jun 28, 2018

ghost commented Jun 28, 2018

emillon Jun 29, 2018

ghost Jun 29, 2018

emillon Jun 29, 2018

ghost Jun 29, 2018

emillon Jun 29, 2018

ghost Jun 29, 2018

emillon Jun 29, 2018

ghost Jun 29, 2018

emillon commented Jun 29, 2018

rgrinberg Jun 29, 2018

ghost Jun 29, 2018

rgrinberg Jun 29, 2018

ghost Jun 29, 2018

ghost commented Jun 29, 2018

rgrinberg left a comment

rgrinberg Jun 30, 2018

ghost Jul 1, 2018

rgrinberg commented Jun 30, 2018

ghost commented Jul 1, 2018

rgrinberg commented Jul 2, 2018 via email

ghost commented Jul 2, 2018

Add a default alias #926

Add a default alias #926

Conversation

ghost commented Jun 28, 2018

Definition

Default target

Future work

Implementation details

ghost commented Jun 28, 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

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

emillon commented Jun 29, 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

ghost commented Jun 29, 2018

rgrinberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Jun 30, 2018

ghost commented Jul 1, 2018

rgrinberg commented Jul 2, 2018 via email

ghost commented Jul 2, 2018