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

[WIP] Unified module handling #2305

Closed
wants to merge 19 commits into from
Closed

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented Jun 20, 2019

This PR has a few seemingly separate goals, but they are quite tricky to separate into their own PR's. So instead of spending more time into splitting this into logical chunks, I will try to get it all in at once:

  • Introduce Modules.t as the central type for holding module layout information. This type will allow us to develop several new features on top of it. Examples include: qualified subdirs, name mangling for executables.

  • Replace Lib_modules by Modules.t wherever possible. Whatever use cases remain, can be replaced by Modules.Lib.

  • Use the same dependency graph in a compilation context for all modules. Previously, we'd try to be a little too clever for our own good and use specialized graphs for wrapped compat/alias modules. This is too hard to track in one's head once we have a combinatorial explosion of wrapped compat, vlib, implementations, qualified subdirs.

  • Use a combined module representation for implementations of virtual libraries. This will greatly simplify the dependency handling for virtual libraries and replace a few weird looking use cases of Vimpl.

  • Fix dependency graph for wrapped compat modules. Previously, those would always depend on the module they replace. That's not really correct, as those modules must be go through the lib interface module.

  • Simplify interpretations of ocamlobjinfo deps. Now the compilation units are just looked up directly. There's no need to splice module names.

  • Remove copying code for virtual library implementations. The non copying approach is far simpler to implement with this scheme. We can store a bit of information for every module such as from : Vlib | Impl and extend the Obj_dir.t type to allow for a Impl of { vlib : t ; impl : t } constructor.

  • Make stdlib a different layout in Modules.t. Currently, this is being special cased in many places of the codebase. Yes, this is quite a special case, but we might as well add first class support for the compiler and the stdlib

  • Refactor building alias modules outside of lib_rules. We'd like this functionality to be available for executables as well. This is also a pre-requesite for the stdlib stuff to work.

One thing that this PR made me realize is that virtual libraries with wrapped_compat are basically broken. This means that it's going to be quite difficult to transition unwrapped vlibs to wrapped mode. @avsm @TheLortex is this a problem in practice? It should be possible to fix after this PR.

@rgrinberg rgrinberg requested a review from a user June 20, 2019 13:51
@rgrinberg rgrinberg requested a review from fpottier as a code owner June 20, 2019 13:51
@rgrinberg rgrinberg force-pushed the cc-modules branch 2 times, most recently from 88f3d3c to 7db671d Compare June 20, 2019 20:52
@rgrinberg
Copy link
Member Author

@emillon think you can give this an initial review while Jeremie is away? The point of this PR is to simplify a bunch of things, so hopefully it should be accessible to all contributors. There are some rough edges that need polish, but the main interface is established.

@rgrinberg rgrinberg force-pushed the cc-modules branch 2 times, most recently from 68487b9 to 6ecff9f Compare June 22, 2019 17:01
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.

There's a lot going on so it's a bit difficult for me to review but this looks like a useful abstraction to have.

@rgrinberg
Copy link
Member Author

The latest commit changes the format of the dune-package file. This only works well if dune is not a {build} dependency of packages.

@rgrinberg rgrinberg closed this Jun 24, 2019
@rgrinberg rgrinberg reopened this Jun 24, 2019
@rgrinberg
Copy link
Member Author

I'm thinking that we'll need a new interface for accessing the alias module of other modules. Something like:

val alias_module : Modules.t -> Module.t -> Module.t option

This will allow us to have unified interface stdlib, wrapped, and qualified subdirs.

@rgrinberg rgrinberg force-pushed the cc-modules branch 3 times, most recently from 4b4c101 to 0838d7e Compare June 25, 2019 05:03
rgrinberg added 15 commits June 25, 2019 18:27
This helper isn't so useful

Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
The code is now simpler and doesn't special case these modules in anyway

Signed-off-by: Rudi Grinberg <[email protected]>
This function only needs cm_files

Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
this isn't necessary anymore as the list of modules looked up is merged
with the virtual library

Signed-off-by: Rudi Grinberg <[email protected]>
This information is directly in the module

Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Move its functionality to Modules.t

Signed-off-by: Rudi Grinberg <[email protected]>
Replace Lib_modules everywhere. There's still no support for the stdlib
layout

Signed-off-by: Rudi Grinberg <[email protected]>
Previously, we'd rely on complex module name splicing. Now we just care
about the compilation units.

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg
Copy link
Member Author

To make it easier on reviewers, I've put some effort into splitting this PR after all. The change to simplify reading ocamlobjinfo deps has already been merged. Now, I will extract the minimum required to fix the dependency graph for wrapped compat modules.

@ghost
Copy link

ghost commented Jul 1, 2019

Thanks, making review easy is important :)

@rgrinberg rgrinberg closed this Jul 4, 2019
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jul 18, 2019
CHANGES:

- Don't select all local implementations in `dune utop`. Instead, let the
  default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex,
  review by @rgrinberg)

- Check that selected implementations (either by variants or default
  implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by
  @rgrinberg)

- Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml)

- Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The
  set of variants is now computed when the virtual library is installed.
  Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex,
  review by @diml)

- Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275,
  @rgrinberg)

- Fix error message for missing C++ sources. The `.cc` extension was always
  ignored before. (ocaml/dune#2275, @rgrinberg)

- Add `$ dune init project` subcommand to create project boilerplate according
  to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder)

- Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo)

- Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review
  by @rgrinberg and @diml)

- Add a `cinaps` extension and stanza for better integration with the
  [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269,
  @diml)

- Allow to embed build info in executables such as version and list
  and version of statically linked libraries (ocaml/dune#2224, @diml)

- Set version in `META` and `dune-package` files to the one read from
  the vcs when no other version is available (ocaml/dune#2224, @diml)

- Add a variable `%{target}` to be used in situations where the context
  requires at most one word, so `%{targets}` can be confusing; stdout
  redirections and "-o" arguments of various tools are the main use
  case; also, introduce a separate field `target` that must be used
  instead of `targets` in those situations.  (ocaml/dune#2341, @aalekseyev)

- Fix dependency graph of wrapped_compat modules. Previously, the dependency on
  the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg)

- Allow to promote executables built with an `executable` stanza
  (ocaml/dune#2379, @diml)

- When instantiating an implementation with a variant, make sure it matches
  virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322,
  @TheLortex, review by @rgrinberg)

- Add a variable `%{ignoring_promoted_rules}` that is `true` when
  `--ingore-promoted-rules` is passed on the command line and false
  otherwise (ocaml/dune#2382, @diml)

- Fix a bug in `future_syntax` where the characters `@` and `&` were
  not distinguished in the names of binding operators (`let@` was the
  same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml)

- Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325,
  @rgrinberg)

- Improve opam generation to include the `dune` dependncies with the minimum
  constraint set based on the dune language version specified in the
  `dune-project` file. (2383, @avsm)

- The order of fields in the generated opam file now follows order preferred in
  opam-lib. (@avsm, ocaml/dune#2380)

- Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384)

- Add warning `66` to default set of warnings starting for dune projects with
  language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299)

- Add (dialect ...) stanza
  (@nojb, ocaml/dune#2404)

- Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412)

- Do not warn about merlin files pre 1.9. This warning can only be disabled in
  1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jul 22, 2019
CHANGES:

- Don't select all local implementations in `dune utop`. Instead, let the
  default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex,
  review by @rgrinberg)

- Check that selected implementations (either by variants or default
  implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by
  @rgrinberg)

- Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml)

- Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The
  set of variants is now computed when the virtual library is installed.
  Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex,
  review by @diml)

- Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275,
  @rgrinberg)

- Fix error message for missing C++ sources. The `.cc` extension was always
  ignored before. (ocaml/dune#2275, @rgrinberg)

- Add `$ dune init project` subcommand to create project boilerplate according
  to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder)

- Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo)

- Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review
  by @rgrinberg and @diml)

- Add a `cinaps` extension and stanza for better integration with the
  [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269,
  @diml)

- Allow to embed build info in executables such as version and list
  and version of statically linked libraries (ocaml/dune#2224, @diml)

- Set version in `META` and `dune-package` files to the one read from
  the vcs when no other version is available (ocaml/dune#2224, @diml)

- Add a variable `%{target}` to be used in situations where the context
  requires at most one word, so `%{targets}` can be confusing; stdout
  redirections and "-o" arguments of various tools are the main use
  case; also, introduce a separate field `target` that must be used
  instead of `targets` in those situations.  (ocaml/dune#2341, @aalekseyev)

- Fix dependency graph of wrapped_compat modules. Previously, the dependency on
  the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg)

- Allow to promote executables built with an `executable` stanza
  (ocaml/dune#2379, @diml)

- When instantiating an implementation with a variant, make sure it matches
  virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322,
  @TheLortex, review by @rgrinberg)

- Add a variable `%{ignoring_promoted_rules}` that is `true` when
  `--ingore-promoted-rules` is passed on the command line and false
  otherwise (ocaml/dune#2382, @diml)

- Fix a bug in `future_syntax` where the characters `@` and `&` were
  not distinguished in the names of binding operators (`let@` was the
  same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml)

- Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325,
  @rgrinberg)

- Improve opam generation to include the `dune` dependncies with the minimum
  constraint set based on the dune language version specified in the
  `dune-project` file. (2383, @avsm)

- The order of fields in the generated opam file now follows order preferred in
  opam-lib. (@avsm, ocaml/dune#2380)

- Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384)

- Add warning `66` to default set of warnings starting for dune projects with
  language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299)

- Add (dialect ...) stanza
  (@nojb, ocaml/dune#2404)

- Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412)

- Do not warn about merlin files pre 1.9. This warning can only be disabled in
  1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon)

- Add a new `inline_tests` field in the env stanza to control inline_tests
  framework with a variable (ocaml/dune#2313, @mlasson, original idea by @diml, review
  by @rgrinberg).

- New binary kind `js` for executables in order to explicitly enable Javascript
  targets, and a switch `(explicit_js_mode)` to require this mode in order to
  declare JS targets corresponding to executables. (ocaml/dune#1941, @nojb)
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.

2 participants