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

Allow include in install stanza #6139

Merged
merged 4 commits into from
Oct 12, 2022
Merged

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Sep 8, 2022

Closes #256

@gridbugs
Copy link
Collaborator Author

gridbugs commented Sep 9, 2022

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.

Copy link
Collaborator

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

Just one suggestion to make header title case

@@ -1413,6 +1413,37 @@ installed in. If the section above is documented as "with the executable bit
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Globbing requires less escaping, that might read better:

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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/* ')'")

Include { context; path } )
]
in
decode_binding <|> decode_include
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

@@ -67,22 +67,3 @@ cryptic and can involve unrelated files:
-> required by _build/default/indirect/a.exe
-> required by alias indirect/indirect-deps in indirect/dune:6
[1]

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Alizter commented Oct 2, 2022

Please can you add a changelog?

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

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
(deps (source_tree resources))
(action
(with-stdout-to resources.sexp
(system "echo \"($(ls -1 resources | xargs -I{} echo '(resources/{} as resources/{})'))\""))))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean by OCaml source here?

Copy link
Member

Choose a reason for hiding this comment

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

I mean write some print_dir.ml

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

and execute it with (run ocaml print_dir.ml)

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

Choose a reason for hiding this comment

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

might as well inline this alias

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to clarify, do you mean changing decode to:

    let decode =
      let open Dune_lang.Decoder in
      repeat decode

Copy link
Member

Choose a reason for hiding this comment

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

More simply: Dune_lang.Decoder.repeat decode.

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"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

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.

@gridbugs gridbugs force-pushed the install-include branch 4 times, most recently from 1f092ad to 3874bbf Compare October 11, 2022 23:09
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 rgrinberg merged commit fd03cce into ocaml:main Oct 12, 2022
@rgrinberg rgrinberg added this to the 3.5.0 milestone Oct 12, 2022
emillon added a commit to emillon/opam-repository that referenced this pull request Oct 19, 2022
…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 gridbugs deleted the install-include branch October 11, 2023 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use :include in install stanza
5 participants