-
Notifications
You must be signed in to change notification settings - Fork 409
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
Introduce a strict_package_deps field #3117
Conversation
In the WIP commit, I've attempted to implement this feature when resolving closures for libraries and executables. Then, I've realized that it's much simpler to just reuse the inferred package dependencies from the build system. However, the initial approach leads to better error messages and I have some other uses for plumbing the package field of libraries. So I will rework that commit to keep the plumbing. |
ce98616
to
bae4dc3
Compare
Another interesting feature to add: Dune could also infer package dependencies of external libraries. For example, if a public library |
0ce16e3
to
ce8b88a
Compare
I've removed the attempts at detecting the packages of external paths. I'll try this again with a less general approach in However, I am thinking that perhaps we should think of a better default policy for this feature. What about the following: If you are using This means that upgrading from 2.2 to 2.3 could technically break things, but I feel like it's quite easy to disable and the feature seems desirable and harmless in 99% of cases. |
@aalekseyev the implementation is ready for review. The parts about attaching |
35a1a5e
to
8944a3e
Compare
@aalekseyev I've removed the unnecessary code. @avsm what do you think about my comment? I thought about things a bit more and I tend to agree with you that this option is essentially always desirable and making it off by default will just introduce churn for downstream users. |
8944a3e
to
21d4e9a
Compare
I think breakage when you bump a minor language version is not a big deal, but I hear that our users expect stronger guarantees. @dim said that we should discuss on discuss before changing the policy to generally allow breakage on minor language version bump. (although perhaps this one is OK on a case-by-case basis, as you say) (the PR as it currently is is not introducing the breakage because strict package deps default to false) |
Signed-off-by: Rudi Grinberg <[email protected]>
21d4e9a
to
2ef0ab4
Compare
For now let's be conservative and just make it a separate mode. We could enable it in 2.4 if we change our mind. |
…lugin, dune-private-libs and dune-glob (2.3.0) CHANGES: - Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes ocaml/dune#3046, @shonfeder) - `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103, fixes ocaml/dune#3088, @shonfeder) - Avoid linear walk to detect children, this should greatly improve performance when a target has a large number of dependencies (ocaml/dune#2959, @ejgallego, @aalekseyev, @Armael) - [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of Coq's stdlib (ocaml/dune#3096, @ejgallego) - [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego) - Better error reporting for "data only" and "vendored" dirs. Using these with anything else than a strict subdirectory or `*` will raise an error. The previous behavior was to just do nothing (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos) - Fix bootstrap on bytecode only switches on windows or where `-j1` is set. (ocaml/dune#3112, @xclerc, @rgrinberg) - Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690 @voodoos) - Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them to be used to fail (ocaml/dune#3138, @rgrinberg) - Introduce a `strict_package_deps` mode that verifies that dependencies between packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117)
…lugin, dune-private-libs and dune-glob (2.3.0) CHANGES: - Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes ocaml/dune#3046, @shonfeder) - `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103, fixes ocaml/dune#3088, @shonfeder) - Avoid linear walk to detect children, this should greatly improve performance when a target has a large number of dependencies (ocaml/dune#2959, @ejgallego, @aalekseyev, @Armael) - [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of Coq's stdlib (ocaml/dune#3096, @ejgallego) - [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego) - Better error reporting for "data only" and "vendored" dirs. Using these with anything else than a strict subdirectory or `*` will raise an error. The previous behavior was to just do nothing (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos) - Fix bootstrap on bytecode only switches on windows or where `-j1` is set. (ocaml/dune#3112, @xclerc, @rgrinberg) - Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690 @voodoos) - Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them to be used to fail (ocaml/dune#3138, @rgrinberg) - Introduce a `strict_package_deps` mode that verifies that dependencies between packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117) - Make sure the `@all` alias is defined when no `dune` file is present in a directory (ocaml/dune#2946, fix ocaml/dune#2927, @diml)
I thought a bit more about this question of breakages in minor release of the language. When it comes to errors, in the end it seems fine to me to immediately get more error when you bump the minor language version. i.e. you bump the minor version and you start getting more errors because Dune got more clever. That seems fine because it's not changing what things mean, we just detect a bit more error cases, which is always beneficial to the user. However, we have to make sure that there are no false negatives. i.e. that we don't report as errors things that are correct. For this PR, because we don't interpret optional dependencies, it seems that we could have false negative. So making it opt-in seems like the right thing to do. |
…lugin, dune-private-libs and dune-glob (2.3.0) CHANGES: - Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes ocaml/dune#3046, @shonfeder) - `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103, fixes ocaml/dune#3088, @shonfeder) - Avoid linear walk to detect children, this should greatly improve performance when a target has a large number of dependencies (ocaml/dune#2959, @ejgallego, @aalekseyev, @Armael) - [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of Coq's stdlib (ocaml/dune#3096, @ejgallego) - [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego) - Better error reporting for "data only" and "vendored" dirs. Using these with anything else than a strict subdirectory or `*` will raise an error. The previous behavior was to just do nothing (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos) - Fix bootstrap on bytecode only switches on windows or where `-j1` is set. (ocaml/dune#3112, @xclerc, @rgrinberg) - Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690 @voodoos) - Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them to be used to fail (ocaml/dune#3138, @rgrinberg) - Introduce a `strict_package_deps` mode that verifies that dependencies between packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117) - Make sure the `@all` alias is defined when no `dune` file is present in a directory (ocaml/dune#2946, fix ocaml/dune#2927, @diml)
In this mode, dune makes sure that all the package dependencies that it
infers are present in the opam file