RFC: add `(strict_includes)` in `(library)` and `(executable)` by hnrgrgr · Pull Request #430 · ocaml/dune

hnrgrgr · 2018-01-21T14:03:25Z

Sometimes, in order to enforce abstractions, we might want to build a library without allowing access to all the .cmi of the "recursive" libraries it depends upon. For instance, we might want to call the OCaml compiler with only the -I of the explicitly listed libraries and not the -I of their recursives closures.

This patch allows this behaviour by introducing an optional (strict_includes) stanza in (library) and (executables).

The default behaviour is not changed and obviously this patch preserves the recursive closure of dependent libraries when calling the executable linkers.

ghost · 2018-01-21T14:34:24Z

Thanks, that seems like a reasonable feature.

A few remarks before looking at the code:

I don't really like talking about include directories in jbuild files, it feels a bit low-level. Maybe we could have (strict_libraries (foo bar)) instead of (libraries (foo bar)) to get the strict behavior
Does the (strict_include) has an affect in the following jbuild file:

(library
 ((name a)
  (modules (a))))

(library
 ((name b)
  (modules (b))
  (libraries (a))))

(library
 ((name c)
  (modules (c))
  (libraries (b))))

(library
 ((name d)
  (modules (d))
  (strict_includes)
  (libraries (c))))

? Given that all the cmi will end up in the same directory, I assume no, which is not good. #427 would solve this issue, would you consider implementing it? :)

hnrgrgr · 2018-01-21T15:14:49Z

I don't really like talking about include directories in jbuild files

Agreed.

Given that all the cmi will end up in the same directory, I assume no, which is not good. #427 would solve this issue, would you consider implementing it? :)

You are right. I will have a look into #427, but probably not before the end of the week.

hnrgrgr · 2018-01-21T23:15:18Z

I found some time earlier than expected for prototyping #427, I would appreciate your feedback.

The current patch only moves the OCaml objects (Foo_a.{cmi,cmo,cmx,o}) into .foo.objs, the library files (foo.{cma,cmxa,a}) and others stubs are still in their current place.

Regarding the 'strict' inclusion of library, I tried to gave it more thoughs and the disjunction between (libraries) and (strict_libraries) puzzles me and I am not sure of the best user interface. Just for quick prototyping I implemented a third solution (libraries (a !b c)) where a and c behaves as usual and onlye the library b has the 'strict' attributes. The same syntax is also used in lib.requires.sexp, this allows the 'strict' attribute to be propagated (I don't have a specific usage in mind but it seems somewhat more coherent).

ghost · 2018-01-22T07:25:44Z

I found some time earlier than expected for prototyping #427, I would appreciate your feedback.

Cool, thanks. I'd rather not rely on symlinks as on Windows this will cause one more file copy. Does it work if we just change the compiler invocation to:

$ ocamlc ... -o src/.foo.objs/blah.cmo

The current patch only moves the OCaml objects (Foo_a.{cmi,cmo,cmx,o}) into .foo.objs, the library files (foo.{cma,cmxa,a}) and others stubs are still in their current place.

That should be fine.

Regarding the 'strict' inclusion of library, I tried to gave it more thoughs and the disjunction between (libraries) and (strict_libraries) puzzles me and I am not sure of the best user interface. Just for quick prototyping I implemented a third solution (libraries (a !b c)) where a and c behaves as usual and onlye the library b has the 'strict' attributes. The same syntax is also used in lib.requires.sexp, this allows the 'strict' attribute to be propagated (I don't have a specific usage in mind but it seems somewhat more coherent).

So, how does it work, do libraries essentially have two sets of dependencies? API ones and link time ones? Note that this is actually what we do inside Jane Street, however we do it automatically: the API dependencies are computed by reading the output of ocamlobjinfo. Though this requires being able to associate modules to libraries.

samoht · 2018-01-22T09:01:58Z

do libraries essentially have two sets of dependencies? API ones and link time ones

I think this would be a sane default: you can only use symbols in the libraries defined in the libraries field. I am not even sure we need to keep the current behaviour at all.

hnrgrgr · 2018-01-22T09:08:35Z

I'd rather not rely on symlinks as on Windows this will cause one more file copy. Does it work if we just change the compiler invocation to: $ ocamlc ... -o src/.foo.objs/blah.cmo

That's what the patch does, it currently invokes the compiler the way you proposed.

The symlink is the other way round (e.g. src/blah.cmo references src/.foo.objs/blah.cmo). and it is not used by default. I added the symlink in a second time, just to preserves the compatibility with invocation like jbuilder build src/blah.cmo, which are used in the blockbox test (easily fixable) but also in external project (like Janestreet's base). This symlink things is only implemented in the third patch of the series and can be easily removed.

hnrgrgr · 2018-01-22T09:14:30Z

do libraries essentially have two sets of dependencies? API ones and link time ones

Sort of, the API one being included in the link time one. In term of implementation, I just added a boolean in Lib.t, stating whether the libraries should be in the 'include path' or not.

type t =
  | Internal of Internal.t * bool
  | External of Findlib.package * bool

I think this would be a sane default: you can only use symbols in the libraries defined in the libraries field. I am not even sure we need to keep the current behaviour at all.

That makes sense, and would simplify the implementation.

bobot · 2018-01-22T09:18:33Z

do libraries essentially have two sets of dependencies? API ones and link time ones

I think this would be a sane default: you can only use symbols in the libraries defined in the libraries field. I am not even sure we need to keep the current behaviour at all.

If the behavior you propose is to only see modules that are in libraries defined in the libraries field, it removes the possibility to define libraries that regroup other libraries or dually to cut a library in sub-part. The namespace proposal could better solve this problem, but currently I think we should still have the default of recursive dependency.

samoht · 2018-01-22T09:47:14Z

it removes the possibility to define libraries that regroup other libraries or dually to cut a library in sub-part

I am not sure to understand. In both cases you can define new module aliases, and that should just work, right?

bobot · 2018-01-22T10:09:46Z

it removes the possibility to define libraries that regroup other libraries or dually to cut a library in sub-part

I am not sure to understand. In both cases you can define new module aliases, and that should just work, right?

If you have A that is just defining aliases to B (A.B), you need to see a.cmi and b.cmi in order to access A.B.t. So if there is no recursion you need to use (libraries a b) and not only (libraries a). The namespace proposal adds, I think, the recursion on the file-system so that we don't need to have it at the library level.

ghost · 2018-01-22T11:11:15Z

That's what the patch does, it currently invokes the compiler the way you proposed.

The symlink is the other way round (e.g. src/blah.cmo references src/.foo.objs/blah.cmo). and it is not used by default. I added the symlink in a second time, just to preserves the compatibility with invocation like jbuilder build src/blah.cmo, which are used in the blockbox test (easily fixable) but also in external project (like Janestreet's base). This symlink things is only implemented in the third patch of the series and can be easily removed.

Ah, I see. Sorry, I didn't read the patch carefully enough. That makes sense.

ghost · 2018-01-22T11:13:32Z

I'm a bit lost in all the discussions, can someone post a jbuild file defining several libraries with some use of the new syntax and explain what libraries are expected to be seen by each library?

hnrgrgr · 2018-01-22T13:07:51Z

Here is the current semantics and syntax:

(library
 ((name a)
  (libraries (lwt))
  (modules (a))))

;; Default behaviour (unchanged)
(library
 ((name b)
  (modules (b)) ;; Sees `a`, `lwt` (and its dependecies)
  (libraries (a))))

;; Importing `b` without visibility on its dependencies
(library
 ((name c)
  (modules (c)) ;; Only sees `b`
  (libraries (!b))))

;; By default, importing `c` makes `b` visible, but not `a` nor `lwt`...
(library
 ((name d)
  (modules (d)) ;; Sees `b` and `c`
  (libraries (c))))

;; Explicitly importing `b` allows to make `a` and `lwt` visible...
(library
 ((name e)
  (modules (e)) ;; Sees `a`, `b`, `c`, `d, `lwt`,...
  (libraries (b d))))

ghost · 2018-01-22T13:16:49Z

OK, thanks. BTW are there cases where we want the strict behavior only for a subset of the dependencies listed in the libraries field? i.e. do we expect users to ever write: (libraries (!a b))?

About changing the default, you can try but I'm pretty sure that this will break tons of stuff.

ghost · 2018-01-22T13:29:09Z

Wait, in your example, shouldn't d see b as well?

hnrgrgr · 2018-01-22T13:29:23Z

BTW are there cases where we want the strict behavior only for a subset of the dependencies?

I am not sure. The patch was simpler this way and I am not against using (libraries) and (strict_libraries), I just choosed the simpler syntax for quick prototyping.

Thanks for your feedbacks.

hnrgrgr · 2018-01-22T13:30:19Z

Wait, in your example, shouldn't d see b as well?

It does, I fixed the comment.

ghost · 2018-01-22T13:55:42Z

OK, so in the new world, is writing !x good and writing x bad? For instance writing !core is never a good idea as you would need to write !core !core_kernel !base even if you only refer to Core in your code, which is weird.

ghost · 2018-01-22T13:58:49Z

For instance, I was trying to think of this feature as:

!x is for dependencies used only in .mli files
x is for all other dependencies

but that still doesn't work for Core.

ghost · 2018-01-22T14:55:02Z

BTW, I guess Core is not the common case. If it help with real use cases, I'm fine with the feature as it is now.

ghost · 2018-01-23T05:34:00Z

I thought a bit more about this, it seems to me that:

the strict mode is better in general
it is only an issue for libraries that extend other ones

Based on that I suggest:

to make the strict behavior a setting that one might set globally for a project, this could fit in the new environment feature (Environment & build profiles #419)
to allow libraries to express that they extend another library, so that for instance adding core to the list of dependencies is the same as adding core base core_kernel, even when the strict behavior is on

Then in a distant future we could try to make the strict behavior the default.

What do you think?

ghost · 2018-01-31T18:03:07Z

@hnrgrgr, could you extract the implementation of #427 into a separate PR? This will allow to make progress on some other PRs until a consensus has been reached for this one.

hnrgrgr · 2018-02-01T00:23:42Z

@diml Rebased the patch and made anew PR, see #472.

ghost · 2018-02-01T09:44:50Z

Great, thanks!

ghost · 2018-02-19T08:25:23Z

I was thinking about this again and it seems to me that the best would be a compiler feature to express that we want to reexport a module. i.e. something like this:

module X = Foo.X [@@reexport]
include Y [@@reexport]

that would effectively inline the cmi contents instead of just creating aliases

rgrinberg · 2018-08-23T12:34:01Z

@Octachron doesn't that make sense? If a client code is relying on some library to introduce some type equalities (or A.t not being abstract in this case - if I'm understanding this right), then they should introduce a dependency on the library of a.ml explicitly.

Octachron · 2018-08-23T13:02:58Z

I imagine that asking users to do some dependency analysis on their own in those corner cases is sensible, but this is also a warning that there is observable differences between building with and without the transitive closure of dependencies. Moreover, hiding type equalities do not always result in errors: Hiding an equality t=float can silently impede some of the adhoc float optimisation for instance.

ghost · 2018-08-23T13:29:29Z

Technically, it doesn't have to break the optimization right? I mean, if a type Y.t is declared as type t = X.t and you know that X.t is a float, you could record that fact when compining Y.

Overall, I feel like they will be corner cases, however we should be able to solve them. Providing this feature and using it will allow us to discover them.

Octachron · 2018-08-23T13:45:49Z

Sure, the cmi representation could be changed to record the full type declaration and hide it, rather than link to the original definition like currently. I rather agree with your analysis, at most it could be nice to document those known corner cases.

ghost · 2018-08-23T14:08:01Z

Documenting them seems right

rgrinberg · 2018-09-01T10:30:41Z

@hnrgrgr if you don't have time, we can help out with rebasing this work as well. Just let us know.

rgrinberg · 2018-09-03T03:30:19Z

@diml I'm wondering if the project itself is actually the correct place to set this mode. I assume that we'd start the transition to this new mode, and eventually, we'd make this the default. In which case this option would be obsolete. At the moment, I'm fairly convinced that the current recursive behavior is useless apart from backwards compatibility concerns. So I think it would be more trouble than its worth to make an option that would eventually removed available in so many places.

Extending libraries can be handled with a field like exported_libraries. This field will default to the empty list, but would be set for core_kernel in core for example. I think that there would be no need to set base here, as I'd expect that core_kernel would already export it. I'd expect that this particular field would indeed be recursive. The only issue I see is that we'd only make it possible to define external grouped libraries like this in dune. That seems acceptable.

For instance, I was trying to think of this feature as:

!x is for dependencies used only in .mli files

x is for all other dependencies

but that still doesn't work for Core.

This concern seems orthogonal to me. Nevertheless, it seems useful to be able to control interface dependencies this way. How about adding an ordered set valued interface_libraries field?. :standard in this case mean would mean to use the set of dependencies in the libraries field unioned with the closure of exported_libraries. In any case, we don't have to do this level of control right away.

ghost · 2018-09-03T11:53:35Z

It would indeed be nice to make it the default in (lang dune 2.0). However, I don't know yet how realistic that is, we might find more issues on the way. It seems to me that the most important is to able to enable it for a given project. We shouldn't worry about core_kernel, I expect that we should be able to solve the problem for core_kernel without adding anything in dune.

That is how I expect things to go:

we add support only for the transitive_deps field (we drop all the other ideas proposed in this PR)
projects that want to try it add (env (_ (transitive_deps hidden))) in their toplevel dune file
as expected, we realize that it is a better default and all the actively developed projects naturally migrate to it

Then when we are ready to release (lang dune 2.0) we have the choice between the following:

if at this point it is obvious that (transitive_deps hidden) is always better and there are no more cases where it cannot be used, we simply drop this field in (lang dune 2.X) and always hide transitive dependencies for projects using (lang dune 2.X)
if it's clear that it is a better default but there are still cases where we can't use (transitive_deps hidden), we simply flip the default from visible to hidden
if it's not clear that it is a better default, we change nothing

rgrinberg · 2018-09-03T12:10:05Z

Alright, that sounds like a good idea. Btw, I think that rebasing this PR is not realistic - far too much has changed. I guess I could try writing this myself if Gregoire is preoccupied.

hnrgrgr · 2018-09-03T12:27:08Z

I also agree with the proposal. I won't be able to work on it before October. Feel free to rebase/rewrite, if it is more urgent.

ghost · 2018-09-03T13:21:47Z

Noted. Just in case we work on this before October, @hnrgrgr could you sign off the current commits and force-push?

rgrinberg · 2018-09-03T20:48:31Z

I think it's unlikely that I'd get to work on this before the ICFP as well. Feel free to take your time.

Sometimes, in order to enforce abstractions, we might want to build a library without allowing access to all the `.cmi` of the 'recursive' libraries it depend upon. For instance, we might want to call the OCaml compiler with only the `-I` of the explicitly listed libraries and not the `-I` of their recursives closures. This patch allow this behaviour by introducing an optional `!` as first character of a dependent library. The default behaviour is not changed and obviously this patch preserves the recursive closure of dependent libraries when calling the executable linkers.

lpw25 · 2018-09-20T22:32:20Z

Sorry for coming to this very late, but I'm not sure this is the right approach to this issue. Basically, there are two kinds of lookups in the type environment:

Looking up the path and definition associated with a particular string.
Looking up a definition based on a given path.

For example, when you write:

type t = Foo.t

in a file bar.mli you will do a lookup of the first kind for the string Foo when compiling bar.mli whereas users of bar.cmi my well do a lookup of the second kind for the path Foo.

Transitive dependencies are a bad thing for lookups of the first kind: you only want the user to be able to write Foo in their file if they actually specified the library containing Foo as a dependency.

However, for lookups of the second kind the compiler always assumes that the transitive closure of dependencies will be available -- and which lookups it does is essentially an implementation detail that the user has no real control over.

(A third kind of lookup is looking up a .cmx file for a path -- but this is really just a variation on the second kind of lookup.)

So if you provide the transitive dependencies you get extra names in scope, whereas if you don't you get unexpected errors.

With the compiler as it is now the only known workaround for this issue is to have the build system -open a module that shadows the .cmi files which should be visible as paths but not as strings. This is an ugly hack and doesn't give great error messages.

The best solution would be to change how the compiler works. All that is needed is a distinction between include paths that should be available for the both kinds of lookup and include paths that should only be available for the second kind. I think that this would be fairly easy to add now thanks to @diml's recent changes to how the initial environment is represented.

bobot · 2018-12-13T20:51:17Z

Perhaps it changed from previous version, but @garrigue was advising the hiding of cmi in this mantis ticket https://caml.inria.fr/mantis/view.php?id=6823 .

rgrinberg · 2019-01-08T19:39:36Z

I revived this in #1745

@diml

CHANGES: - Second step of the deprecation of jbuilder: the `jbuilder` binary now emits a warning on every startup and both `jbuilder` and `dune` emit warnings when encountering `jbuild` files (ocaml/dune#1752, @diml) - Change the layout of build artifacts inside _build. The new layout enables optimizations that depend on the presence of `.cmx` files of private modules (ocaml/dune#1676, @bobot) - Fix merlin handling of private module visibility (ocaml/dune#1653 @bobot) - unstable-fmt: use boxes to wrap some lists (ocaml/dune#1608, fix ocaml/dune#1153, @emillon, thanks to @rgrinberg) - skip directories when looking up programs in the PATH (ocaml/dune#1628, fixes ocaml/dune#1616, @diml) - Use `lsof` on macOS to implement `--stats` (ocaml/dune#1636, fixes ocaml/dune#1634, @xclerc) - Generate `dune-package` files for every package. These files are installed and read instead of `META` files whenever they are available (ocaml/dune#1329, @rgrinberg) - Fix preprocessing for libraries with `(include_subdirs ..)` (ocaml/dune#1624, fix ocaml/dune#1626, @nojb, @rgrinberg) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - When executing actions, open files lazily and close them as soon as possible in order to reduce the maximum number of file descriptors opened by Dune (ocaml/dune#1635, ocaml/dune#1643, fixes ocaml/dune#1633, @jonludlam, @rgrinberg, @diml) - Reimplement the core of Dune using a new generic memoization system (ocaml/dune#1489, @rudihorn, @diml) - Replace the broken cycle detection algorithm by a state of the art one from [this paper](https://doi.org/10.1145/2756553) (ocaml/dune#1489, @rudihorn) - Get the correct environment node for multi project workspaces (ocaml/dune#1648, @rgrinberg) - Add `dune compute` to call internal memoized functions (ocaml/dune#1528, @rudihorn, @diml) - Add `--trace-file` option to trace dune internals (ocaml/dune#1639, fix ocaml/dune#1180, @emillon) - Add `--no-print-directory` (borrowed from GNU make) to suppress `Entering directory` messages. (ocaml/dune#1668, @dra27) - Remove `--stats` and track fd usage in `--trace-file` (ocaml/dune#1667, @emillon) - Add virtual libraries feature and enable it by default (ocaml/dune#1430 fixes ocaml/dune#921, @rgrinberg) - Fix handling of Control+C in watch mode (ocaml/dune#1678, fixes ocaml/dune#1671, @diml) - Look for jsoo runtime in the same dir as the `js_of_ocaml` binary when the ocamlfind package is not available (ocaml/dune#1467, @nojb) - Make the `seq` package available for OCaml >= 4.07 (ocaml/dune#1714, @rgrinberg) - Add locations to error messages where a rule fails to generate targets and rules that require files outside the build/source directory. (ocaml/dune#1708, fixes ocaml/dune#848, @rgrinberg) - Let `Configurator` handle `sizeof` (in addition to negative numbers). (ocaml/dune#1726, fixes ocaml/dune#1723, @Chris00) - Fix an issue causing menhir generated parsers to fail to build in some cases. The fix is to systematically use `-short-paths` when calling `ocamlc -i` (ocaml/dune#1743, fix ocaml/dune#1504, @diml) - Never raise when printing located errors. The code that would print the location excerpts was prone to raising. (ocaml/dune#1744, fix ocaml/dune#1736, @rgrinberg) - Add a `dune upgrade` command for upgrading jbuilder projects to Dune (ocaml/dune#1749, @diml) - When automatically creating a `dune-project` file, insert the detected name in it (ocaml/dune#1749, @diml) - Add `(implicit_transitive_deps <bool>)` mode to dune projects. When this mode is turned off, transitive dependencies are not accessible. Only listed dependencies are directly accessible. (ocaml/dune#1734, ocaml/dune#430, @rgrinberg, @hnrgrgr) - Add `toplevel` stanza. This stanza is used to define toplevels with libraries already preloaded. (ocaml/dune#1713, @rgrinberg) - Generate `.merlin` files that account for normal preprocessors defined using a subset of the `action` language. (ocaml/dune#1768, @rgrinberg) - Emit `(orig_src_dir <path>)` metadata in `dune-package` for dune packages built with `--store-orig-source-dir` command line flag (also controlled by `DUNE_STORE_ORIG_SOURCE_DIR` env variable). This is later used to generate `.merlin` with `S`-directives pointed to original source locations and thus allowing merlin to see those. (ocaml/dune#1750, @andreypopp) - Improve the behavior of `dune promote` when the files to be promoted have been deleted. (ocaml/dune#1775, fixes ocaml/dune#1772, @diml) - unstable-fmt: preserve comments (ocaml/dune#1766, @emillon) - Pass flags correctly when using `staged_pps` (ocaml/dune#1779, fixes ocaml/dune#1774, @diml) - Fix an issue with the use of `(mode promote)` in the menhir stanza. It was previously causing intermediate *mock* files to be promoted (ocaml/dune#1783, fixes ocaml/dune#1781, @diml) - unstable-fmt: ignore files using OCaml syntax (ocaml/dune#1784, @emillon) - Configurator: Add `which` function to replace the `which` command line utility in a cross platform way. (ocaml/dune#1773, fixes ocaml/dune#1705, @Chris00) - Make configurator append paths to `$PKG_CONFIG_PATH` on macOS. Previously it was prepending paths and thus `$PKG_CONFIG_PATH` set by users could have been overridden by homebrew installed libraries (ocaml/dune#1785, @andreypopp) - Disallow c/cxx sources that share an object file in the same stubs archive. This means that `foo.c` and `foo.cpp` can no longer exist in the same library. (ocaml/dune#1788, @rgrinberg) - Forbid use of `%{targets}` (or `${@}` in jbuild files) inside preprocessing actions (ocaml/dune#1812, fixes ocaml/dune#1811, @diml) - Add `DUNE_PROFILE` environment variable to easily set the profile. (ocaml/dune#1806, @rgrinberg) - Deprecate the undocumented `(no_keep_locs)` field. It was only necessary until virtual libraries were supported (ocaml/dune#1822, fix ocaml/dune#1816, @diml) - Rename `unstable-fmt` to `format-dune-file` and remove its `--inplace` option. (ocaml/dune#1821, @emillon). - Autoformatting: `(using fmt 1.1)` will also format dune files (ocaml/dune#1821, @emillon). - Autoformatting: record dependencies on `.ocamlformat-ignore` files (ocaml/dune#1824, fixes ocaml/dune#1793, @emillon)

Summary: Before D38213812 if a target *t* depended on a library *a* where *a* depends on a library *b*, then the modules of *b* were accessible to *t*. In D38213812 we changed this behavior so that in order to use the modules of *b* from *t*, then *b* must be in *t*'s `deps` (that is, a "direct" dependency). Why? williewillus [noted](https://fb.workplace.com/groups/403065016408786/permalink/8781197225262148/) buck2 differed from buck1 in this respect, admitting transitive use where buck1 did not. I argued then that we wrote the buck2 build rules that way with the understanding we were overcoming a buck1 shortcoming. However opinion prevailed that the buck1 behavior was "better". For example, mroch argued "I was able to depend on `String_utils` in `Server_files_js`, because `Server_files_js` depends on `Sys_utils` and `Sys_utils` depends on `String_utils`. That's an implementation detail! I should have to explicitly depend on things I use directly, not get them for free because something else used them transitively". We found cause to reconsider this restriction. Consider this program: ``` let (): unit = Core.Printf.printf "Hello world!" ``` The dune file for the above program would look something like this: ``` (executable (name core-test) (libraries core) ) ``` When we translate that into a buck2 target we get the following. ``` ocaml_binary( name = "core-test", srcs = ["core_test.ml"], external_deps = [ ("supercaml", None, "core"), ], ) ``` Before this diff (in the presence of the no transitive deps rule) building this target results in an error: ``` 1 | let (): unit = Core.Printf.printf "Hello world!" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Error: This expression has type Core__.Import.unit = Base.Unit.t but an expression was expected of type unit ``` The issue is that for this to work there needs to be a `-I` for `Base` on the compile line and we deliberately prune it out! So, this is a fix ``` ocaml_binary( name = "core-test", srcs = ["core_test.ml"], external_deps = [ ("supercaml", None, "core"), ("supercaml", None, "base"), ], ) ``` but it's not satisfying and potentially hard to discover. If on the other hand we relax the no transitive deps rule, the build "just works" as it does with dune. Given this example, as samwgoldman put it, adding `-I`'s for transitive dependencies is the "lesser of two evils". *Note: There are ongoing discussions in the community about compiler support for solving the problems we are grappling with here. See e.g. ocaml/dune#430 (comment) and ocaml/RFCs#31 Reviewed By: samwgoldman Differential Revision: D43504245 fbshipit-source-id: ccb70c4b6c8e700287e1c5eb8eff0677a8b7f855

Summary: Before D38213812 (d872b98) if a target *t* depended on a library *a* where *a* depends on a library *b*, then the modules of *b* were accessible to *t*. In D38213812 (d872b98) we changed this behavior so that in order to use the modules of *b* from *t*, then *b* must be in *t*'s `deps` (that is, a "direct" dependency). Why? williewillus [noted](https://fb.workplace.com/groups/403065016408786/permalink/8781197225262148/) buck2 differed from buck1 in this respect, admitting transitive use where buck1 did not. I argued then that we wrote the buck2 build rules that way with the understanding we were overcoming a buck1 shortcoming. However opinion prevailed that the buck1 behavior was "better". For example, mroch argued "I was able to depend on `String_utils` in `Server_files_js`, because `Server_files_js` depends on `Sys_utils` and `Sys_utils` depends on `String_utils`. That's an implementation detail! I should have to explicitly depend on things I use directly, not get them for free because something else used them transitively". We found cause to reconsider this restriction. Consider this program: ``` let (): unit = Core.Printf.printf "Hello world!" ``` The dune file for the above program would look something like this: ``` (executable (name core-test) (libraries core) ) ``` When we translate that into a buck2 target we get the following. ``` ocaml_binary( name = "core-test", srcs = ["core_test.ml"], external_deps = [ ("supercaml", None, "core"), ], ) ``` Before this diff (in the presence of the no transitive deps rule) building this target results in an error: ``` 1 | let (): unit = Core.Printf.printf "Hello world!" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Error: This expression has type Core__.Import.unit = Base.Unit.t but an expression was expected of type unit ``` The issue is that for this to work there needs to be a `-I` for `Base` on the compile line and we deliberately prune it out! So, this is a fix ``` ocaml_binary( name = "core-test", srcs = ["core_test.ml"], external_deps = [ ("supercaml", None, "core"), ("supercaml", None, "base"), ], ) ``` but it's not satisfying and potentially hard to discover. If on the other hand we relax the no transitive deps rule, the build "just works" as it does with dune. Given this example, as samwgoldman put it, adding `-I`'s for transitive dependencies is the "lesser of two evils". *Note: There are ongoing discussions in the community about compiler support for solving the problems we are grappling with here. See e.g. ocaml/dune#430 (comment) and ocaml/RFCs#31 Reviewed By: samwgoldman Differential Revision: D43504245 fbshipit-source-id: ccb70c4b6c8e700287e1c5eb8eff0677a8b7f855

hnrgrgr force-pushed the strict_includes branch from 8bf0d42 to 34b5df1 Compare January 21, 2018 15:02

hnrgrgr force-pushed the strict_includes branch 2 times, most recently from c9d3072 to ee0d33c Compare January 21, 2018 22:10

hnrgrgr force-pushed the strict_includes branch from 25c5994 to 4ec65b9 Compare January 22, 2018 01:24

ghost mentioned this pull request Jan 31, 2018

Import configurator #467

Merged

4 tasks

hnrgrgr added 3 commits September 5, 2018 12:24

WIP ocaml#427

39f61ce

WIP: compatibility layer

c1ee1d3

hnrgrgr force-pushed the strict_includes branch from 4ec65b9 to c1ee1d3 Compare September 5, 2018 10:38

hnrgrgr requested a review from a user September 5, 2018 10:38

hnrgrgr requested review from emillon, hhugo and rgrinberg as code owners September 5, 2018 10:38

rgrinberg mentioned this pull request Sep 11, 2018

Dune should warn about dependencies on unused libraries #650

Closed

rgrinberg mentioned this pull request Jan 8, 2019

Strict Includes - Attempt 2 #1745

Merged

1 task

rgrinberg closed this Jan 8, 2019

rgrinberg mentioned this pull request Feb 12, 2019

[new release] dune (1.7.0) ocaml/opam-repository#13444

Merged

Conversation

hnrgrgr commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 21, 2018

Uh oh!

hnrgrgr commented Jan 21, 2018

Uh oh!

hnrgrgr commented Jan 21, 2018

Uh oh!

ghost commented Jan 22, 2018

Uh oh!

samoht commented Jan 22, 2018

Uh oh!

hnrgrgr commented Jan 22, 2018

Uh oh!

hnrgrgr commented Jan 22, 2018

Uh oh!

bobot commented Jan 22, 2018

Uh oh!

samoht commented Jan 22, 2018

Uh oh!

bobot commented Jan 22, 2018

Uh oh!

ghost commented Jan 22, 2018

Uh oh!

ghost commented Jan 22, 2018

Uh oh!

hnrgrgr commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 22, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 22, 2018

Uh oh!

hnrgrgr commented Jan 22, 2018

Uh oh!

hnrgrgr commented Jan 22, 2018

Uh oh!

ghost commented Jan 22, 2018

Uh oh!

ghost commented Jan 22, 2018

Uh oh!

ghost commented Jan 22, 2018

Uh oh!

ghost commented Jan 23, 2018

Uh oh!

ghost commented Jan 31, 2018

Uh oh!

hnrgrgr commented Feb 1, 2018

Uh oh!

ghost commented Feb 1, 2018

Uh oh!

ghost commented Feb 19, 2018

Uh oh!

rgrinberg commented Aug 23, 2018

Uh oh!

Octachron commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

Octachron commented Aug 23, 2018

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

rgrinberg commented Sep 1, 2018

Uh oh!

rgrinberg commented Sep 3, 2018

Uh oh!

ghost commented Sep 3, 2018

Uh oh!

rgrinberg commented Sep 3, 2018

Uh oh!

hnrgrgr commented Sep 3, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 3, 2018

Uh oh!

hnrgrgr commented Jan 21, 2018 •

edited

Loading

hnrgrgr commented Jan 22, 2018 •

edited

Loading

ghost commented Jan 22, 2018 •

edited by ghost

Loading

Octachron commented Aug 23, 2018 •

edited

Loading

hnrgrgr commented Sep 3, 2018 via email •

edited

Loading