fix hooks and plugins usage when cleaning build artifacts for deps

[cjkjellander]

We are using a dependency that compiles a NIF, but it never clean up its .so-files.

I tried following this example:
https://github.com/blt/port_compiler

And also:
https://www.rebar3.org/v3/docs/building-cc

Turns out, hooks or plugins are never run during clean for any dep.

find_apps didn't read config files so no hooks were in the app_infos,
and now that hooks are being done rebar needs plugins to be able
to run clean plugin hooks in deps.

[tsloughter]

Hm, could you include a test? Pretty sure I understand what the issue is but maybe the how the fix work will make more sense to me with a test.

[tsloughter]

Like why is install_deps needed as a dep for clean?

[tsloughter]

Oh and did you run rebar3 clean -a?

I assumed the issue was that the deps weren't including their hooks in their config when running clean and thus each app had an empty hook list. But instead is it that you are running rebar3 clean and it isn't cleaning the deps at all?

[cjkjellander]

Sure, I'll try and make a test.

The reason why install_deps there is because of dep rebar.configs like this one:

{provider_hooks,
 [
  {pre,
   [
    {compile, {pc, compile}},
    {clean, {pc, clean}}
   ]
  }
 ]
}.

app_discovery does not load all provider_hooks needed for clean into the #state{}, but install_deps does. What is the intent of having a clean provider_hook? Doesn't the provider_hook need to be installed for rebar3 to properly be able to run clean?

Anyway, if clean doesn't depend on install_deps, a lot of builds out in the wild will fail since the bugfix for loading in the config for the hooks and actually trying to run them without actually having the hook beams in the system with fail. So i opted to try and not break peoples builds.

I the way I figured that out was that as soon as I fixed the first bug, our builds started failing in the repo I was using the new rebar3 code.

[ferd]

The problem with using install_deps in as a dependency of clean is that you now need to download dependencies before removing them.

[cjkjellander]

I agree, but how are you going to run the clean hook if it isn't downloaded?

So what is the intent of a clean provider_hook, should it:

1. always run, and by that also download the hook so it can run?
2. silently ignore the hook if it isn't downloaded yet?
3. try and run the hook, crash, and fail the clean?

I'm gonna add a test case, I have it ready.

[cjkjellander]

That test fails on master, but not in my branch.

[cjkjellander]

I'm gonna see if fails on my first commit as well, cause if it passes on my first commit, the test does not test deep enough.

[cjkjellander]

Oh, and to answer tsloughter's question, yes, we run rebar3 clean -a.

[cjkjellander]

Ok, the test is deep enough, if you remove the dependency install_deps from clean the test fails.

[cjkjellander]

I fixed a typo in the commit message.

[cjkjellander]

I can't figure out why it failed a test in a random module that I haven't touched.

[cjkjellander]

is it possible to rerun the travis?

[ferd]

I haven't given it a lot of thought (still coming back from holidays) but I would expect that silently ignore the hook if it isn't downloaded yet? would probably make sense in this case, but I'm not 100% sure, since in other situations I wouldn't do the same and would expect a failure.

Like there's a very clear expectation that we don't require to compile the beam files just to remove them either, so downloading deps just to get rid of them is weird. Additionally what we're needing to build here specifically are not deps, but plugins. Right now they're all intertwined though.

Like this just sounds like we've messed up the design for rebar3 clean and there's no good fix for it at first sight.

[cjkjellander]

So this is what I have discovered so far. app_discovery doesn't discover any plugins, and find_all_apps didn't even read in the rebar.config files of the apps so no hooks at all were in the list, it was always []. In fact as I followed the #state{} through both compile and clean the state was empty of all plugins and hooks after app_discovery but had everything in the state after install_deps, and when I fixed the first bug of not reading rebar.config our builds started failing since now the hook was there, but not the actual code for the hook. rebar_prv_clean has always tried to run hooks for deps, but the list of hooks was always [] before.

We could make app_discovery load code that is installed like install_deps does. But that seemed like a lot of copy paste to me, and the two are not structured in the same way at all so I couldn't figure out an easy way to do it.

We could do a separate load_plugins provider, and try to rip that part out of install_dep. Again, I couldn't figure out an easy way to do it.

The reason I did the one-liner was that it actually makes the examples of port_compile and Compiling C/C++ start to actually work. As it was they only work for top level apps and not for deps at all.

[ferd]

So there's a lot here

• I believe app_discovery only discovers top-level apps and puts them in the path. It fetches the config and specs for plugins, locks, and the top-level deps, but nothing transitive
• install_deps does fetch the dependencies and plugins and maintains them in state
• the compile step dynamically unloads plugins (but keeps deps around) for the compilation itself to avoid polluting paths of the compiler. When it comes to the part where we generate the .app file, we re-load the libs in path, and then drop them again once we're done before re-running hooks
• the state can be modified or different on a per-app basis, since each app may have local configs that are specific to it and not inherited from the top level

I think we have juggled with some ideas in the past, one being a 'discover_deps' step that discovers currently installed deps and plugins and loads them, but does not install them if they're not there yet. I think that was in the context of allowing offline builds though, so I'm not sure what is appropriate here.

Top-level plugins should however be in path and accessible at this point, since those should be fetched on any run, even if only to call rebar3 help. My expectation would be that rebar3 clean works fine with top-level apps even with plugins from that point of view.

I however am not surprised if the dependencies' own plugins do not work with rebar3 clean -a since the perspective of dependencies is not really seen there. clean is a bit like compile in that it has two hookable levels: one for the entire run, and one per app (or dependency). The section on hooks on the docs website explains the distinction: https://www.rebar3.org/v3/docs/configuration#section-hookable-points-in-providers

Before fixing the behaviour of clean with dependencies, we have to be clear on which context it runs for. There's a definite need to fix the per-dep cleanup. I'm not sure the current solution is appropriate though. Especially since to clean up local applications (without -a) we end up downloading dependencies that were possibly not even fetched yet. It might be simpler to do things like call install_deps by hand in that clause, but I think we may have to revisit that discover_deps provider for local runs since it could be of use here.

[cjkjellander]

I still think this should be merged until a better solution is coded. It is the path of least surprise. Sure if you clone a new repo and do a clean, it will download the deps first. Confusing, maybe, but it won't break your build.

As it stands with this bug in place, it actually breaks your build and in our case leaves .so files that shouldn't be there after a clean. In our company we use both Mac and Linux machines, but all our servers run Linux, and when we compile inside docker we want all Mac compiled .so files to be gone so they can be compiled for Linux inside the docker container. This of course doesn't happen, and we have to work around it with Makefiles doing the cleaning.

[cjkjellander]

I think that 1. always run, and by that also download the hook so it can run? is actually the path of least surprise. While fixing some bugs for port_compiler I had deleted the plugins dir and then figured out I needed to do a 'rebar3 clean -a'. Of course it downloaded pc and did the clean without any problems. Why do we need to crash?

I still think this should be merged as is so we can fix the bug until we have a better solution.

[ferd]

Sorry, life has been super hectic for me lately and I haven't had enough time to give to all of this.

Unless @tsloughter has oppositions to revert this, I'll merge the pull request since I've had no time to spend on it and the current patch, while unintuitive, at least allows proper cleanup to take place for build steps.

As such I'll be including it as a bug fix and we can always revisit the behaviour later for a more optimal running mechanism. Those who want to get rid of the deps can still use rm -rf _build as they likely do today.

[cjkjellander]

Great news!

However, I think we should put up a release note if we can. There are lots of repos out there that use hooks that never have been run. I was hit by this one this morning: https://github.com/deadtrickster/prometheus.erl/blob/084616996c4cd6d499ade1fd925dbbdfb88cac90/rebar.config#L22

I was building this as a dep inside docker using ubuntu:xenial and it turns out that that post hook needs 'ed' and that wasn't installed. So it might be confusing for people that we fix this bug.

So where do we do release notes?

[ferd]

I'll put them when we cut a release.

But yeah, if that breaks too many builds, then we're gonna be in a tough stop of breaking backwards compat by fixing bugs that turned out to be essential in making things work.

We'll probably want to make the notice very visible, or otherwise take it up on ourselves to go and submit patches to well-known libraries that are known to break before we cut the next release.