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

Update compiler hooks order and internal compiler phases #2211

Merged
merged 3 commits into from
Jan 25, 2020

Conversation

ferd
Copy link
Collaborator

@ferd ferd commented Jan 24, 2020

Phase 1 of the Compiler Refactoring Extravaganza: #2200

Preliminary benchmarks showed no noticeable performance regression (nor improvement).
Biggest risk IMO is the possibility that some projects relied on hook order being all done for one app before the next one begins in order to build and compile.

For example, imagine we have 2 top-level apps: A and B. A relies on B, and B uses post-compilation hooks to provide artifacts that A requires to successfully compile. The change of ordering here will break this dependency chain, for which we have no means of fixing.
Any other hooks usage should be right.

All tests (both systests and ct tests) pass fine, but I am nevertheless a bit worried about introducing breaking changes in larger projects. We should possibly come up with a list of those to check before merging, and ask for a release-candidate for the next release to prevent breakage.

ferd added 3 commits January 22, 2020 21:48
This allows breaking apart the pre-hooks from the rest of the
compilation steps, as a preliminary step towards being able to do some
analysis on all project apps at once before actually compiling them.
This is a new way to orient the code and would allow better
parallelization or multi-app scan phases
Copy link
Collaborator

@tsloughter tsloughter left a comment

Choose a reason for hiding this comment

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

Merging to master gets it into nightly builds which are easy to ask people to try because we have binaries for them in s3.

@ferd
Copy link
Collaborator Author

ferd commented Jan 25, 2020

Yeah, longer time in nightly is probably safer.

@ferd ferd merged commit d82846c into erlang:master Jan 25, 2020
@michalmuskala
Copy link

This results in very strange output when using escriptize hook in a multi-app setup.

{provider_hooks, [{post, [{compile, escriptize}]}]}.

It will first compile everything and then print a lone Building escript... without any reference into what escript that is - before when order was preserved it was easier to understand.

I'm not sure if there is similar behaviour in other providers.

@ferd
Copy link
Collaborator Author

ferd commented Apr 18, 2020

yeah, that's fair. Might be worth clarifying the escript message. Probably all providers' logs (especially those often used in hooks) could benefit from being more explicit now that the hook order changed.

ferd added a commit to ferd/rebar3 that referenced this pull request May 18, 2020
Since the compiler refactor changed the order of steps and all hooks get
applied at once, knowing which app is being escriptized isn't really
obvious, particularly when many apps individually define their own 'main
app' and they get run at once.

This adds the name of the main app being run when escriptizing it to make
it clear what goes on, especially if there are failures.

This was reported in erlang#2211 (comment)
@nalundgaard
Copy link

@ferd @tsloughter Quick question on this one.

I maintain a library that has a pre-compile hook in the rebar.config that executes an escript to read some JSON templates and use them to dynamically generate Erlang modules in the src/generated dir. This escript has a header like this:

#!/usr/bin/env escript
%% -*- erlang -*-
%%! -smp enable -sname typegen -pa ebin -pa deps/jiffy/ebin -pa _build/default/lib/jiffy/ebin -pz ../jiffy/ebin
%% Note: the pa/pz madness above are to make this script work in a rebar2, rebar3 (direct), and rebar3 (as dep) context.

The escript uses jiffy to decode the JSON files and process them into Erlang modules that implement a behavior. This has worked as recently as rebar 3.13.2, but I recently updated a downstream application to 3.14.1, and now I am getting this error when running rebar3 compile:

escript: exception error: undefined function jiffy:decode/2
  in function  lists:map/2 (lists.erl, line 1239)
===> Hook for compile failed!

When I look into the _build/default/lib/jiffy/ebin directory in my downstream app, there are no beam files there, only a rendered jiffy.app file. It sure looks like this is the result of making the pre-compile hook for the dependentt library run before the compilation of its own (recursive) dependencies. Have you guys considered this use case? Is there something I am doing wrong here, or a way to tell rebar that I need to generate these source files before compiling the application, but after compiling my dependent applications?

@ferd
Copy link
Collaborator Author

ferd commented Oct 9, 2020

Can you show how you specify hooks?

There is obviously the risk that you're the one edge case I was afraid could happen in that you rely on the explicit "pre-hook for A runs after full compilation for B has completed." If so -- and it seems likely this is your case -- I don't really see a good way to move forwards with this since we had this commit merged in January, and as release candidates from April to the last day of August, and the new behaviour is generally more efficient and more correct. I don't really think we can afford to roll that back right now.

The one thing I could recommend is writing a plugin (http://rebar3.org/docs/tutorials/building_plugins/) or a custom compiler for JSON files specifically (http://rebar3.org/docs/extending/custom_compiler_modules/ -- the doc is too minimal and if you're interested I'll expand it), but these formats will unfortunately not work well for rebar 2.

If that seems tricky or too painful, there might be a messy workaround in declaring jiffy as a plugin, adding _build/default/plugins/jiffy/ebin to the paths in the escript, and seeing if it's now in path outside of the deps path and included as a plugin before hooks are run.

@nalundgaard
Copy link

Here's what it looks like:

{pre_hooks, [
    {compile, "./typegen"}
]}.

Where typegen is the escript file I mentioned above.

It looks like the 'messy' workaround of using jiffy as a plugin with the -pa refs in the escript do work as expected, but then you (and all apps that depend on you) get the nice warning 1-2 times:

./rebar3 eunit
===> Plugin jiffy does not export init/1. It will not be used.
===> Plugin jiffy does not export init/1. It will not be used.

Certainly that can work in a pinch, but I'd rather find another alternative here—maybe storing these as consult-able Erlang term file rather than JSON (basically so it can be read from built in file:consult/1 instead of jiffy or jsx)? That would work, but the files would sure be unnecessarily ugly.

I am not sure this process would lend itself to being transformed into a compiler module, since all the json files in a directory are validated in aggregate. That is, they form a network of interconnected structures that must be checked for cycles, dead refs, etc. that necessitates parsing all, validating them together, then rendering them into Erlang modules using the aggregate context built during validation. It looks like the compiler module has some sense of choosing some files to go first, but in essence handles a file at a time, if I understand it right?

In terms of the plugin suggestion, I think this escript's functionality could be fit into a plugin, but I think that wouldn't alter the issue per se: I would need to bundle my own JSON decoder into the plugin, right? It doesn't seem like a plugin can depend on a library that isn't a plugin.

It seems like all 3 of these options basically entail copying/writing a JSON decoder into the rebar3 plugin infrastructure to get that Erlang code into the context prior to application compilation. Is that basically correct, or is there some other way I am not seeing?

@ferd
Copy link
Collaborator Author

ferd commented Oct 9, 2020

The compiler modules do operate on a per-file basis, but do a big analysis phase at first that builds a DAG of changed or modified files to know what needs re-building and in which order. If you generate an aggregate .erl file with no clear way to do it partially or in sections, then it may be overly tricky.

That a plugin relies on libraries that are only in plugin space is a feature done on purpose in this case. The plugin dependency will be built on startup and used only in that context. This is desired when the plugin has no reason to ship with a built app. For example, it also allows code analytic plugins to run whatever they need without requiring to agree with what the user app's requires.

So you're right that in this case, it entails making a JSON decoder into the plugin structure. If they're a dependency, they won't show the warning message you saw on your run (we only show this for plugins declared as plugins)

@nalundgaard
Copy link

nalundgaard commented Oct 10, 2020

Yeah, I agree with the general plugin structure and how it avoids shipping code for building in the release, although in my case jiffy is always shipping with our releases, so it's a moot point in this use case. It would be interesting to allow plugins to depend on libraries that aren't themselves plugins for use cases like this (and keep those plugin deps isolated from the app deps).

If they're a dependency, they won't show the warning message you saw on your run (we only show this for plugins declared as plugins)

I assume this means if I were to 'plugin-ize' a JSON decoder like jiffy/jsx by implementing the rebar3 plugin callbacks, I could use it as both a plugin and a dependency? That seems like the "right" way to do this whether I plugin-ize my pre-compile escript or not—even in that case, a JSON decoder in plugin space seems like it should really be its own entity. Is that right, or is there some potential concern for errors trying to load conflicting modules from the plugins and lib directories in any stage of the rebar3 life cycle?

@tsloughter
Copy link
Collaborator

@nalundgaard if you were to go that far why not make the prehook a provider instead of an escript?

A plugin can depend on any dependencies it wants, so depending on jiffy would be fine in the plugin.

@nalundgaard
Copy link

A plugin can depend on any dependencies it wants, so depending on jiffy would be fine in the plugin.

Ah, I didn't realize that. Great! That sounds like the way to go when we shed the need for rebar2 compatibility internally later this year. Is there a way to package the provider in the same git repository, given that its scope is fundamentally a subset of the library itself, or is separate repositories a necessary part of the architecture?

@ferd
Copy link
Collaborator Author

ferd commented Oct 10, 2020

As for now you need a separate repo.

The one exception to this is if you use something like rebar3_path_deps as a custom resource type specifying where to find the actual plugin, and it would mostly work from the top-level of the project more than as a dep.

Another thing is that since the latest release we also support declaring deps like:

{deps, [
  {rebar, {git_subdir, "git://github.com/erlang/rebar3.git", {branch, "main"}, "subdir"}},
  {rebar, {git_subdir, "git://github.com/erlang/rebar3.git", {tag, "3.14"}, "sub/dir"},
  {rebar, {git_subdir, "git://github.com/erlang/rebar3.git", {ref, "aeaefd"}, "dir"}
]}.

Which would let you fetch a subdirectory of a monorepo into another application over git.

nalundgaard added a commit to nalundgaard/rebar3 that referenced this pull request May 25, 2021
…7c6addfc7ffb8b511 for rebar3 3.13.x.

Per https://ferd.ca/you-ve-got-to-upgrade-rebar3.html:

> [The patch](erlang@17848f4#diff-4b5af60b80cd23cfd32adbf1b5e7647c8d89a8d7046c89cee2be5a5ae14dc95fR14) is not that complex on new versions and should be relatively easy to port to older versions if anyone ends up wanting to help.

This should work for anyone on rebar3 3.13.2 either due to running an older OTP or still needing the old pre-compile hook behavior (erlang#2211 (comment)).
nalundgaard added a commit to nalundgaard/rebar3 that referenced this pull request May 25, 2021
Based on erlang#2561, an adaptation of erlang/rebar3@751fa91 for rebar3 3.13.x.

Per https://ferd.ca/you-ve-got-to-upgrade-rebar3.html:

> [The patch](erlang@17848f4#diff-4b5af60b80cd23cfd32adbf1b5e7647c8d89a8d7046c89cee2be5a5ae14dc95fR14) is not that complex on new versions and should be relatively easy to port to older versions if anyone ends up wanting to help.

This should work for anyone on rebar3 3.13.2 either due to running an older OTP or still needing the old pre-compile hook behavior (erlang#2211 (comment)).
nalundgaard added a commit to nalundgaard/rebar3 that referenced this pull request May 26, 2021
Based on erlang#2561, an adaptation of erlang/rebar3@751fa91 for rebar3 3.13.x.

Per https://ferd.ca/you-ve-got-to-upgrade-rebar3.html:

> [The patch](erlang@17848f4#diff-4b5af60b80cd23cfd32adbf1b5e7647c8d89a8d7046c89cee2be5a5ae14dc95fR14) is not that complex on new versions and should be relatively easy to port to older versions if anyone ends up wanting to help.

This should work for anyone on rebar3 3.13.2 either due to running an older OTP or still needing the old pre-compile hook behavior (erlang#2211 (comment)).
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.

4 participants