Allow include in install stanza by gridbugs · Pull Request #6139 · ocaml/dune

gridbugs · 2022-09-08T16:23:25Z

Closes #256

gridbugs · 2022-09-09T08:26:36Z

There was a failing test because this removes the dependency cycle reported in #4345, where copying a file from a parent directory with the copy_files stanza in a public library caused a dependency cycle. I've removed the test because it looks like this change accidentally fixes the bug reported in that issue.

christinerose

Just one suggestion to make header title case

christinerose · 2022-09-10T11:33:50Z

doc/dune-files.rst

 set", they are installed with mode ``0o755`` (``rwxr-xr-x``); otherwise they are
 installed with mode ``0o644`` (``rw-r--r--``).

+Including files in the install stanza


Suggested change

Including files in the install stanza

Including Files in the Install Stanza

emillon · 2022-09-12T12:37:51Z

doc/dune-files.rst

+     (deps (source_tree resources))
+     (action
+      (with-stdout-to foo.sexp
+       (system "echo \"($(ls resources))\""))))


Globbing requires less escaping, that might read better:

Suggested change

(system "echo \"($(ls resources))\""))))

(run echo "(" "resources/*" ")"))))

also it's probably more correct since ls resources omits the resources/ prefix

The run action won't work here because it runs the program directly rather than going via a shell, so globs aren't expanded. I agree that globbing is more readable and correct. Changing to (system "echo '(' resources/* ')'")

emillon · 2022-09-12T12:43:18Z

src/dune_rules/dune_file.ml

+              Include { context; path } )
+          ]
+      in
+      decode_binding <|> decode_include


I think that it works, but will give a wrong hint in the case where lang dune < 3.5 and the (include) form is found (you can add a test to show that). I believe that the solution to theat is to parse using <|> without any since in the decode_include branch, but to call since depending on the shape of the result. This will correctly point that include is available since 3.5.

(see the diff in file_binding.ml for an example of this pattern)

There's already a test for this in test/blackbox-tests/test-cases/install-include/install-include.t:

File "dune", line 5, characters 14-32: 5 | (files a.txt (include foo.sexp)) ^^^^^^^^^^^^^^^^^^ Error: 'include' is only available since version 3.5 of the dune language. Please update your dune-project file to have (lang dune 3.5).

Does that look like the right error message?

emillon · 2022-09-12T12:46:32Z

test/blackbox-tests/test-cases/reporting-of-cycles.t/run.t

  -> required by _build/default/indirect/a.exe
  -> required by alias indirect/indirect-deps in indirect/dune:6
  [1]
-


What's the fixed message? It's probably better to keep it rather than remove the test case.

This test case no longer results in an error. From reading #4345 I was under the impression that this case shouldn't result in an error, and the dependency cycle was due to a bug in dune. It now occurs to me that we should keep this test around to detect if this bug comes back, so I've added test/blackbox-tests/test-cases/github4345.t

Alizter · 2022-10-02T03:14:07Z

Please can you add a changelog?

src/dune_rules/file_binding.ml

rgrinberg

LGTM. Pleasantly surprised on how cleanly you managed to tack this on.

How about adding this to (dirs ..) as well? Should hopefully be nice and simple.

test/blackbox-tests/test-cases/github4345.t

rgrinberg · 2022-10-08T16:40:40Z

test/blackbox-tests/test-cases/install-include/install-include-list-files.t/dune

+ (deps (source_tree resources))
+ (action
+  (with-stdout-to resources.sexp
+   (system "echo \"($(ls -1 resources | xargs -I{} echo '(resources/{} as resources/{})'))\""))))


Could you write a simple OCaml source for this instead? It's not particularly readable with the shell.

What do you mean by OCaml source here?

I mean write some print_dir.ml

Sys.readdir "resources |> Array.iter (fun x -> ..)

and execute it with (run ocaml print_dir.ml)

test/blackbox-tests/test-cases/install-include/install-include-invalid-file.t

rgrinberg · 2022-10-08T16:46:57Z

src/dune_rules/file_binding.ml

-          [ Pp.text
-              "invalid format, <name> or (<name> as <install-as>) expected"
-          ]
+    let decode_file = decode


might as well inline this alias

Just to clarify, do you mean changing decode to:

let decode = let open Dune_lang.Decoder in repeat decode

More simply: Dune_lang.Decoder.repeat decode.

rgrinberg · 2022-10-08T16:49:52Z

src/dune_rules/dune_file.ml

+          if Path.Build.Set.mem seen path then
+            User_error.raise
+              ~loc:(String_with_vars.loc path_sw)
+              [ Pp.textf "Include loop detected via: %s"


We need to look into generalizing this cycle detection to have the same implementation for all includes.

Agreed. I'll take a look at this tomorrow.

I added a module Recursive_include which adds (include ...) statements to config languages and used it to implement the include statement in the (install ...) stanza and also the (include_dirs ...) field.

gridbugs · 2022-10-10T13:42:07Z

LGTM. Pleasantly surprised on how cleanly you managed to tack this on.

How about adding this to (dirs ..) as well? Should hopefully be nice and simple.

This should already work. I've added a test.

This is to prevent a dependency cycle between memo nodes when including files in the install stanza. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

The issue was a dependency cycle when a public library copies files from a parent directory using copy_files. The test was that a readible error message is produced in that situation, but it's not necessary that this situation result in a dependency cycle at all. This change moves the test case to its own file and now asserts that the case doesn't result in an error. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) CHANGES: - macOS: Handle unknown fsevents without crashing (ocaml/dune#6217, @rgrinberg) - Enable file watching on MacOS SDK < 10.13. (ocaml/dune#6218, @rgrinberg) - Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg) - Add a `runtime_deps` field in the `cinaps` stanza to specify runtime dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg) - Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg) - Extend dune describe to include the root path of the workspace and the relative path to the build directory. (ocaml/dune#6136, @reubenrowe) - Allow dune describe workspace to accept directories as arguments. The provided directories restrict the worskpace description to those directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope) - Add a terminal persistence mode that attempts to clear the terminal history. It is enabled by setting terminal persistence to `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg) - Disallow generating targets in sub direcories in inferred rules. The check to forbid this was accidentally done only for manually specified targets (ocaml/dune#6031, @rgrinberg) - Do not ignore rules marked `(promote (until-clean))` when `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon) - Dune no longer considers .aux files as targets during Coq compilation. This means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter) - Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg) - Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056, @rgrinberg) - Introduce a `dirs` field in the `install` stanza to install entire directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg) - Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg) - Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes ocaml/dune#5945, @rgrinberg) - Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg) - Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine) - Add an `(include <file>)` term to the `include_dirs` field for adding directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993, @gridbugs) - Support `(extra_objects ...)` field in `(executable ...)` and `(library ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs) - Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb) - Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114, fixes ocaml/dune#6103, @emillon) - odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes ocaml/dune#1117, @emillon) - dune install: copy files in an atomic way (ocaml/dune#6150, @emillon) - Add `%{coq:...}` macro for accessing data about the configuration about Coq. For instance `%{coq:version}` (ocaml/dune#6049, @Alizter) - update vendored copy of cmdliner to 1.1.1. This improves the built-in documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon, ocaml/dune#6169, @shonfeder) - The test suite for Coq now requires Coq >= 8.16 due to changes in the plugin loading mechanism upstream (which now uses `Findlib`). - Starting with Coq build language 0.6, theories can be built without importing Coq's standard library by including `(stdlib no)`. (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek) - on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, ocaml/dune#6231, fixes ocaml/dune#5650, fixes ocaml/dune#6226, @emillon) - Added an (aliases ...) field to the (rules ...) stanza which allows the specification of multiple aliases per rule (ocaml/dune#6194, @Alizter) - The `(coq.theory ...)` stanza will now ensure that for each declared `(plugin ...)`, the `META` file for it is built before calling `coqdep`. This enables the use of the new `Findlib`-based loading method in Coq 8.16; however as of Coq 8.16.0, Coq itself has some bugs preventing this to work yet. (ocaml/dune#6167 , workarounds ocaml/dune#5767, @ejgallego) - Allow include statement in install stanza (ocaml/dune#6139, fixes ocaml/dune#256, @gridbugs) - Handle CSI n K code in ANSI escape codes from commands. (ocaml/dune#6214, fixes ocaml/dune#5528, @emillon) - Add a new experimental feature `mode_specific_stubs` that allows the specification of different flags and sources for foreign stubs depending on the build mode (ocaml/dune#5649, @voodoos)

gridbugs requested a review from christinerose as a code owner September 8, 2022 16:23

gridbugs requested a review from rgrinberg September 8, 2022 16:23

gridbugs force-pushed the install-include branch from 75155d4 to 421632b Compare September 9, 2022 13:18

christinerose approved these changes Sep 10, 2022

View reviewed changes

emillon reviewed Sep 12, 2022

View reviewed changes

emillon mentioned this pull request Sep 15, 2022

Allow globbing in the install stanza #6018

Closed

gridbugs force-pushed the install-include branch 2 times, most recently from 00cd135 to 17b57b6 Compare September 28, 2022 01:34

gridbugs force-pushed the install-include branch from 17b57b6 to 259891a Compare October 3, 2022 01:00

Alizter reviewed Oct 3, 2022

View reviewed changes

src/dune_rules/file_binding.ml Outdated Show resolved Hide resolved

gridbugs force-pushed the install-include branch 2 times, most recently from e596ebc to a5414a2 Compare October 4, 2022 06:15

rgrinberg reviewed Oct 8, 2022

View reviewed changes

gridbugs force-pushed the install-include branch from a5414a2 to 43b64a0 Compare October 10, 2022 13:41

gridbugs force-pushed the install-include branch 4 times, most recently from 1f092ad to 3874bbf Compare October 11, 2022 23:09

gridbugs added 4 commits October 11, 2022 18:29

fix: Defer enumerating files to install

8fa33bc

This is to prevent a dependency cycle between memo nodes when including files in the install stanza. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

feature: Allow include in install stanza

e96f962

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

refactor: Factor out common include file logic

6623284

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

rgrinberg force-pushed the install-include branch from 3874bbf to 6623284 Compare October 12, 2022 01:10

rgrinberg merged commit fd03cce into ocaml:main Oct 12, 2022

rgrinberg added this to the 3.5.0 milestone Oct 12, 2022

emillon mentioned this pull request Oct 19, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) ocaml/opam-repository#22317

Merged

gridbugs deleted the install-include branch October 11, 2023 02:30

	Including files in the install stanza
	Including Files in the Install Stanza

	(system "echo \"($(ls resources))\""))))
	(run echo "(" "resources/*" ")"))))

Conversation

gridbugs commented Sep 8, 2022 • edited by emillon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gridbugs commented Sep 9, 2022

Uh oh!

christinerose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alizter commented Oct 2, 2022

Uh oh!

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gridbugs commented Oct 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gridbugs commented Sep 8, 2022 •

edited by emillon

Loading