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

Finer with-accepted-exit-code restrictions #3027

Merged

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Jan 13, 2020

Add a recursive check of the <DSL> under with-accepted-exit-code constructs such that only run, bash, system, chdir, setenv, ignore-<outputs>, with-stdin-from and with-<outputs>-to can appear.

It was previously restricted to a first-child check accepting only run, bash and system.

This fixes #3014

src/dune/action_ast.ml Outdated Show resolved Hide resolved
@voodoos voodoos changed the title Finer with accepted exit code restrictions Finer with-accepted-exit-code restrictions Jan 13, 2020
@voodoos
Copy link
Collaborator Author

voodoos commented Jan 13, 2020

Note: the new tests require a bump of dune_lang's version in order to pass.

if nesting_support then
is_ok t
else
Syntax.Error.since loc Stanza.syntax (2, 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be possible to refactor that so that it uses Syntax.since directly but I don't think the error messages would be as precise.

@emillon emillon requested a review from a user January 15, 2020 09:38
@voodoos voodoos force-pushed the finer-with-accepted-exit-code-restrictions branch from 430bccf to 98668f4 Compare January 20, 2020 14:28
@voodoos
Copy link
Collaborator Author

voodoos commented Jan 20, 2020

(I took advantage of the last rebase to complete the entry about #3041 in the CHANGES.md)

Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

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

LGTM!

doc/concepts.rst Outdated Show resolved Hide resolved
@voodoos voodoos force-pushed the finer-with-accepted-exit-code-restrictions branch from c92fc61 to 98568a0 Compare January 21, 2020 10:38
src/dune/action_ast.ml Outdated Show resolved Hide resolved
src/dune/action_ast.ml Outdated Show resolved Hide resolved
@voodoos voodoos force-pushed the finer-with-accepted-exit-code-restrictions branch from cd52265 to 6d9f81f Compare January 29, 2020 12:15
@rgrinberg rgrinberg force-pushed the finer-with-accepted-exit-code-restrictions branch from 6d9f81f to 5999ae0 Compare February 6, 2020 18:35
Add a recursive check of the <DSL> under with-accepted-exit-code
constructs such that only run, bash, system, chdir, setenv,
ignore-<outputs>, with-stdin-from and with-<outputs>-to can appear.

It was previously restricted to a first-child check accepting only run,
bash and system.

Fix ocaml#3014

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg force-pushed the finer-with-accepted-exit-code-restrictions branch from 5999ae0 to 424e13f Compare February 6, 2020 18:35
@rgrinberg rgrinberg merged commit 12fdacb into ocaml:master Feb 6, 2020
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Feb 6, 2020
…lugin, dune-private-libs and dune-glob (2.2.0)

CHANGES:

- `dune test` is now a command alias for `dune runtest`. This is to make the CLI
  less idiosyncratic (ocaml/dune#3006, @shonfeder)

- Allow to set menhir flags in the `env` stanza using the `menhir_flags` field.
  (ocaml/dune#2960, fix ocaml/dune#2924, @bschommer)

- By default, do not show the full command line of commands executed
  by `dune` when `dune` is executed inside `dune`. This is to make
  integration tests more reproducible (ocaml/dune#3042, @diml)

- `dune subst` now works even without opam files (ocaml/dune#2955, fixes ocaml/dune#2910,
  @fangyi-zhou and @diml)

- Hint when trying to execute an executable defined in the current directory
  without using the `./` prefix (ocaml/dune#3041, fixes ocaml/dune#1094, @voodoos).

- Extend the list of modifiers that can be nested under
  `with-accepted-exit-codes` with `chdir`,  `setenv`, `ignore-<outputs>`,
  `with-stdin-from` and `with-<outputs>-to` (ocaml/dune#3027, fixes ocaml/dune#3014, @voodoos)

- It is now an error to have a preprocessing dependency on a ppx rewriter
  library that is not marked as `(kind ppx_rewriter)` (ocaml/dune#3039, @snowleopard).

- Fix permissions of files promoted to the source tree when using the
  shared cache. In particular, make them writable by the user (ocaml/dune#3043,
  fixes ocaml/dune#3026, @diml)

- Only detect internal OCaml tools with `.opt` extensions. Previously, this
  detection applied to other binaries as well (@kit-ty-kate, @rgrinberg, ocaml/dune#3051).

- Give the user a proper error message when they try to promote into a source
  directory that doesn't exist. (ocaml/dune#3073, fix ocaml/dune#3069, @rgrinberg)

- Correctly build vendored packages in `-p` mode. These packages were
  incorrectly filtered out before. (ocaml/dune#3705, @diml)

- Do not install vendored packages (ocaml/dune#3704, @diml)

- `make` now prints a message explaining the main targets available
  (ocaml/dune#3085, fix ocaml/dune#3078, @diml)

- Add a `byte_complete` executable mode to build programs as
  self-contained bytecode programs
  (ocaml/dune#3076, fixes ocaml/dune#1519, @diml)
mgree pushed a commit to mgree/opam-repository that referenced this pull request Feb 19, 2020
…lugin, dune-private-libs and dune-glob (2.2.0)

CHANGES:

- `dune test` is now a command alias for `dune runtest`. This is to make the CLI
  less idiosyncratic (ocaml/dune#3006, @shonfeder)

- Allow to set menhir flags in the `env` stanza using the `menhir_flags` field.
  (ocaml/dune#2960, fix ocaml/dune#2924, @bschommer)

- By default, do not show the full command line of commands executed
  by `dune` when `dune` is executed inside `dune`. This is to make
  integration tests more reproducible (ocaml/dune#3042, @diml)

- `dune subst` now works even without opam files (ocaml/dune#2955, fixes ocaml/dune#2910,
  @fangyi-zhou and @diml)

- Hint when trying to execute an executable defined in the current directory
  without using the `./` prefix (ocaml/dune#3041, fixes ocaml/dune#1094, @voodoos).

- Extend the list of modifiers that can be nested under
  `with-accepted-exit-codes` with `chdir`,  `setenv`, `ignore-<outputs>`,
  `with-stdin-from` and `with-<outputs>-to` (ocaml/dune#3027, fixes ocaml/dune#3014, @voodoos)

- It is now an error to have a preprocessing dependency on a ppx rewriter
  library that is not marked as `(kind ppx_rewriter)` (ocaml/dune#3039, @snowleopard).

- Fix permissions of files promoted to the source tree when using the
  shared cache. In particular, make them writable by the user (ocaml/dune#3043,
  fixes ocaml/dune#3026, @diml)

- Only detect internal OCaml tools with `.opt` extensions. Previously, this
  detection applied to other binaries as well (@kit-ty-kate, @rgrinberg, ocaml/dune#3051).

- Give the user a proper error message when they try to promote into a source
  directory that doesn't exist. (ocaml/dune#3073, fix ocaml/dune#3069, @rgrinberg)

- Correctly build vendored packages in `-p` mode. These packages were
  incorrectly filtered out before. (ocaml/dune#3705, @diml)

- Do not install vendored packages (ocaml/dune#3704, @diml)

- `make` now prints a message explaining the main targets available
  (ocaml/dune#3085, fix ocaml/dune#3078, @diml)

- Add a `byte_complete` executable mode to build programs as
  self-contained bytecode programs
  (ocaml/dune#3076, fixes ocaml/dune#1519, @diml)
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.

Extend what (with-accepted-exit-codes) supports
4 participants