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

Split up the compiler DAG #2217

Merged
merged 1 commit into from
Feb 5, 2020
Merged

Split up the compiler DAG #2217

merged 1 commit into from
Feb 5, 2020

Conversation

ferd
Copy link
Collaborator

@ferd ferd commented Jan 31, 2020

This is another tricky commit towards replacing the current module
analysis with EPP. The compiler DAG is now being shared across multiple
applications being compiled, rather than a per-application basis, which
promises to allow better ordering, parallelism, and more thorough
invalidation of runtime dependencies when they are modified.

This however required changes:

  • The compiler DAG is no longer private to rebar_compiler, and has
    been extracted to the rebar_compiler_dag module
  • The compiler DAG is now started by the rebar_prv_compile module,
    which oversees the calls to rebar_compiler for each OTP application
  • The compiler DAG has been refactored to use a "dirty flag" to know if
    it was modified, rather than just tracking modifications in a
    functional manner, since the scope change (going multi-app) makes it
    impossible to cleanly use the functional approach without much larger
    changes
  • The DAG used to be cached within each OTP application. This is no
    longer possible since it is shared. Instead the DAG is stored in the
    state's deps_dir, which allows to cleanly split caches between regular
    apps for the user's project and plugins
  • The DAG supported a "label" mode that was used to store distinct DAGs
    for extra_src_dir runs and regular modules; this label is now used
    (and extended to rebar_prv_compile internals) to distinguish between
    "compile runs", such as "project_apps", or just "apps" (deps). The
    label is optional (i.e. not used by plugins which have no such need)
  • The extra_src_dirs for each app is now compiled using the main app's
    DAG, but the run takes place later in the compilation process. This
    may need changing to detect and prevent dependencies from src_dirs
    into extra_src_dirs, but this should not technically be a problem for
    runtime anyway.
  • Reworked the support for extra_src_dirs that are at the root of an
    umbrella project (and therefore do not belong to any single app) to
    use the new structure, also as part of the project_apps DAG.
  • The clean command no longer clears the DAG since it's not part of
    the app itself anymore; the prune step should handle it.
  • I dropped the "include dir changes deletes the DAG" criteria since it
    clearly doesn't make sense anymore. I plan to eventually replace it with
    things such as the compiler version instead.

All tests keep passing, and this puts us in a better place to use EPP
with cross-app support in the near-future.

This is another tricky commit towards replacing the current module
analysis with EPP. The compiler DAG is now being shared across multiple
applications being compiled, rather than a per-application basis, which
promises to allow better ordering, parallelism, and more thorough
invalidation of runtime dependencies when they are modified.

This however required changes:
- The compiler DAG is no longer private to `rebar_compiler`, and has
  been extracted to the `rebar_compiler_dag` module
- The compiler DAG is now started by the `rebar_prv_compile` module,
  which oversees the calls to `rebar_compiler` for each OTP application
- The compiler DAG has been refactored to use a "dirty flag" to know if
  it was modified, rather than just tracking modifications in a
  functional manner, since the scope change (going multi-app) makes it
  impossible to cleanly use the functional approach without much larger
  changes
- The DAG used to be cached within each OTP application. This is no
  longer possible since it is shared. Instead the DAG is stored in the
  state's deps_dir, which allows to cleanly split caches between regular
  apps for the user's project and plugins
- The DAG supported a "label" mode that was used to store distinct DAGs
  for extra_src_dir runs and regular modules; this label is now used
  (and extended to `rebar_prv_compile` internals) to distinguish between
  "compile runs", such as "project_apps", or just "apps" (deps). The
  label is optional (i.e. not used by plugins which have no such need)
- The extra_src_dirs for each app is now compiled using the main app's
  DAG, but the run takes place later in the compilation process. This
  may need changing to detect and prevent dependencies from src_dirs
  into extra_src_dirs, but this should not technically be a problem for
  runtime anyway.
- Reworked the support for extra_src_dirs that are at the root of an
  umbrella project (and therefore do not belong to any single app) to
  use the new structure, also as part of the project_apps DAG.

All tests keep passing, and this puts us in a better place to use EPP
with cross-app support in the near-future.
@ferd ferd requested a review from tsloughter January 31, 2020 18:19
prepare_compiler_env(AppInfo),
lists:foreach(fun({Compiler, G}) ->
run(G, Compiler, AppInfo),
%% TODO: disable default recursivity in extra_src_dirs compiling to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we really merge this without this disabled by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it's what we have today. See the complaints about modules auto-compiled by CT. I just added the note in comments to fix this long-standing bug.

ok
end,
DAGs);
compile_all(Compilers, AppInfo) -> % =< 3.13.0 interface; plugins use this!
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should decide on how many versions back of rebar3 to support and figure out a way to give useful error messages when plugins are outdated.

Doesn't need to be a part of this PR, just bringing it up because this is adding to the already extensive technical debt for plugin support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah. To some extent I have never taken the time to update the rebar3_proper plugin myself so I feel in a bad place to force others to do it as well. It still displays the cool warning every time you run it.

@ferd ferd merged commit a524fe3 into erlang:master Feb 5, 2020
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