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.

Sign up for GitHub

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

Jump to bottom

Disallow ppx dependencies on non-ppx libraries #3039

Merged
merged 13 commits into from
Jan 21, 2020
Merged

Disallow ppx dependencies on non-ppx libraries #3039

merged 13 commits into from
Jan 21, 2020

Conversation

snowleopard
Copy link
Collaborator

Depending on a ppx rewriter library without declaring as a ppx rewriter using (kind ppx_rewriter) annotation is non-reliable, so we add a check for such situations.

There are two potential problems with the current solution:

  • We raise an error in case of such dependency, but could add a warning instead. The problem with warnings is that there is no support for warnings in the library resolution logic at the moment: we don't want to generate warnings for a library that is unused in the end.
  • I had to mark a ppx driver in one of the tests as a ppx rewriter which is probably not what we want. Instead we could allow ppx dependencies on either ppx rewriters or ppx drivers.

@snowleopard snowleopard force-pushed the ppx-dep-check branch 2 times, most recently from eab7940 to 746515b Compare January 20, 2020 18:58
@snowleopard snowleopard changed the title [WIP] Disallow ppx dependencies on non-ppx libraries Disallow ppx dependencies on non-ppx libraries Jan 20, 2020
@snowleopard
Disallow ppx dependencies on non-ppx libraries
93335d1 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Better error message
2734848 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Make the check conditional
1fd8076 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Fix test
c58b836 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Remove some indirection
3901b08 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Bring two dependency checks together
6dafe84 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Simplify
af201d9 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Minor revision
ea0b432 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Drop unused code
78bce7e 
Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard
Make more checks, add comments, update change log
f2937e2 
Signed-off-by: Andrey Mokhov <[email protected]>
@jeremiedimino
Small simplification
fb8d490 
Signed-off-by: Jeremie Dimino <[email protected]>
ghost
ghost approved these changes Jan 21, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good!

@snowleopard
Copy link
Collaborator Author

@diml Thanks for the review and improvements!

@snowleopard snowleopard merged commit 63298a5 into ocaml:master Jan 21, 2020
@snowleopard snowleopard deleted the ppx-dep-check branch January 21, 2020 10:41
@kit-ty-kate kit-ty-kate mentioned this pull request Jan 24, 2020
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Feb 6, 2020
@rgrinberg
[new release] dune-build-info, dune, dune-configurator, dune-action-p…
5d030d1 
…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)
@rgrinberg rgrinberg mentioned this pull request Feb 6, 2020
Merged
mgree pushed a commit to mgree/opam-repository that referenced this pull request Feb 19, 2020
@rgrinberg @mgree
[new release] dune-build-info, dune, dune-configurator, dune-action-p…
8476e6f 
…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
Reviewers
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@snowleopard @jeremiedimino