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

project_plugins are not downloaded/compiled in the order how they are defined in rebar.config #1857

Closed
tothlac opened this issue Aug 16, 2018 · 19 comments

Comments

@tothlac
Copy link
Contributor

tothlac commented Aug 16, 2018

With version: rebar 3.6.1+build.4118.ref940b1c59 on Erlang/OTP 20 Erts 9.3
Having this in my rebar.config:

{project_plugins, [
    plugin2,
    plugin1
]}.

I want plugin2 to be downloaded and compiled first. However, it looks like they are fetched in an alphabetical order. It's actually not a problem if there is a way to force rebar3 to download plugin2 first.

@tothlac
Copy link
Contributor Author

tothlac commented Aug 16, 2018

As I see project_plugins is already in this wrong order when rebar_plugins:project_plugins_install/1 reads it from the State.

@ferd
Copy link
Collaborator

ferd commented Aug 16, 2018

If you need plugin2 to be downloaded and compiled before plugin1, you probably need plugin1 to depend on plugin2.

@tothlac
Copy link
Contributor Author

tothlac commented Aug 16, 2018

I have one plugin (let's call it first_plug) which must be executed before all other plugins.

We have like 20 plugins. About 10 of them are created by us, the other 10 is created by third parties.
They don't have any relation to first_plug, but I want to execute first_plug first.

I can add first_plug as a dependency to the plugins created by us, but what about third party plugins? For example there is a rebar3_lint plugin on hexpm, should I create a pull request into that plugin?

@ferd
Copy link
Collaborator

ferd commented Aug 17, 2018

Well you're talking about compile order there or running order? Compile order, if you need to compile a plugin before another one I expect that this is related to something like include files or macros. If it's a runtime then the download/compile order is unrelated and dictated by the order of declaration in hooks for example.

So I'm not quite sure what the actual issue is here?

@tothlac
Copy link
Contributor Author

tothlac commented Aug 17, 2018

It's actually both, but in this case I'm referring to the execution order of the init functions.
I've created a simple app which reproduces it:

https://github.com/tothlac/rebar_plugin_orders/blob/master/rebar.config

In rebar.config I have:

{project_plugins,
    [
     {rebar_plugin2, {git,"[email protected]:tothlac/rebar_plugin2.git", {branch,"master"}}},
     {rebar_plugin1, {git,"[email protected]:tothlac/rebar_plugin1.git", {branch,"master"}}}
    ]}.

I want the init function of rebar_plugin2 to be executed first. Unfortunately when I run it I can see:

tothlac:rebar_plugin_orders rm -rf _build; rebar3 compile
===> Fetching rebar_plugin1 ({git,
                                     "[email protected]:tothlac/rebar_plugin1.git",
                                     {branch,"master"}})
===> Compiling rebar_plugin1
rebar_plugin1 16 init: 'init'
===> Fetching rebar_plugin2 ({git,
                                     "[email protected]:tothlac/rebar_plugin2.git",
                                     {branch,"master"}})
===> Compiling rebar_plugin2
rebar_plugin2 16 init: 'init'
===> Verifying dependencies...

This is because rebar_state:get(State, {project_plugins, Profile}, []) in rebar_plugins:project_plugins_install/1 gives back this:

rebar_plugins 25 Plugins: '[{rebar_plugin1,
                                {git,
                                    "[email protected]:tothlac/rebar_plugin1.git",
                                    {branch,"master"}}},
                            {rebar_plugin2,
                                {git,
                                    "[email protected]:tothlac/rebar_plugin2.git",
                                    {branch,"master"}}}]'

So it looks like when rebar.config is read, and the contents are copied to the State, the order is modified.
If I change the order of the two plugins in rebar.config, their init functions are still executed in the same order.

@ferd
Copy link
Collaborator

ferd commented Aug 17, 2018

So two bits of info. First, the values are initialized ASAP because plugins for custom resources are needed right now and then (since they can be used to download dependencies).

Second, the alphabetical order is from the way dependencies are fetched as well (conflict resolution for two equal-level dependencies is just using the names for ordering as a tie-breaker to be repeatable).

There is no ordering in the config file that is carrying any meaning right now. I don't think we ever anticipated the dependencies you mention for the ordering there, and I think internally we had to special-case alias because of how it needs to run last to detect override conflicts but that was years after the fact.

In short, we don't have a solution for this yet. I don't know if declaration order is a thing that really makes sense since it would differ from dependency order. For example, if you have transient deps like:

  • plugin_x depends on plugin2 and plugin1 (2 must be initialized before 1)
  • plugin_b depends on plugin1

From the declaration-order mechanism, anyone using multiple plugins would need to be aware of each of their dependencies and manually control their initialization mechanism, which is really tricky to get right.

Can you give specifics about why you need init order respected so that it can inform any decision we make?

@tothlac
Copy link
Contributor Author

tothlac commented Aug 17, 2018

In this case we can't modify the execution of init functions with dependencies between plugins.
Let's say we want to have two plugins: appup_plugin, our_plugin. appup_plugin was created by someone else, we can't add our_plugin as a dependency to this.

The reason why we want this: I have mentioned earlier we had our modification in rebar's code which mirrors repositories based on some configuration. I'm working currently on moving these modifications into a rebar plugin.

We want to mirror not only dependencies but the plugins as well, and dependencies of the plugins.
Let's say appup_plugin has this is in its rebar.config:

{deps, [
    {bbmustache, "1.5.0"},
    string_compat
]}.

And we want to mirror these dependencies as well. So what we want is by the time rebar3 fetches appup_plugin, it should have already called the init function of rebar3_mirroring. There must be a way to force the execution order.
I don't quite understand why it is useful for you to call the init functions in alphabetical order, would not it be enough to just follow the declaration order, and if there are duplicates, just call a delete_dups function?

Of course alphabetical order would be also ok for us, then I need to modify the name of the mirroring plugin which precedes probably all other plugins. Something like : _rebar3_mirroring. That would be always executed first, would not it? What do you think would it work?

@ferd
Copy link
Collaborator

ferd commented Aug 17, 2018

The reason right now why we call the init function right away is that a resource plugin might be used to fetch a dep, so we can't take the time to reorder them.

The reason for alphabetical order is unrelated; it's just to ensure deterministic builds (specifically making sure we don't rely on an implicit declaration order) when dealing with sub-dependencies.

I agree that currently the ordering -- or lack of ability to specify plugin dependencies or ordering is problematic. I think the way to fix it right now would be through the applications tuple (as mentioned in #1858) so that if you specify 4-5 plugins, we could use these tuples to specify the order for them. It would however possibly break custom resource handlers. So that's a problem that we have to think about.

I'm starting to think we should have a different mechanism for plugins and custom resources (which could maybe let us lock plugins eventually). @tsloughter may have ideas as well here.

For mirroring, see #1777 -- we have plans to overhaul the hex mechanism to help with this, but obviously no timeline for now.

@tothlac
Copy link
Contributor Author

tothlac commented Aug 17, 2018

What about the other question?
Using _rebar3_mirrroring as the name for the plugin would always force rebar3 to execute its init function first before everything else?

A built-in mechanism in rebar3 would be good. We need mirroring the hex pkgs and a bit more. We mirror git repos to our atlassian/bitbucket servers, automatically create them when encountering a new repo which has not been mirrored, then during an rebar3 upgrade command fetch the new commits , automatically rebase/merge them (depending on the upgrade strategy specified in the config) against our internal modifications, and create a pull request if the automatic merge/rebase was not successful because of a conflict. This can be useful in a big as sometimes github does not work for an entire day, so the programmers can't do anything on that day. We are planning to opensource the plugin, unfortunately it is not easy.

@ferd
Copy link
Collaborator

ferd commented Aug 18, 2018

I think that yes, it would force it to be first. That should work, but only as long as we don't come up with a better mechanism, where then it might break since it relied on a build behaviour that could change. But that's probably acceptable to you since otherwise it won't build today.

And yeah so far the mirroring we'd look at would only be with hex; for git there's no great plan there.

@tothlac
Copy link
Contributor Author

tothlac commented Aug 21, 2018

I've tried out this solution to move the plugin which alphabetically precedes everything else. It worked actually. So you don't plan to change the order of the execution of the plugin init functions?

@ferd
Copy link
Collaborator

ferd commented Aug 21, 2018

I mean, we don't have active plans as in "features that require it" in mind. I don't think we'd be closed to the idea, but that's a distinction between "what is in the roadmap" and "what we are not going to do".

I think right now the plugin priority was not in the roadmap, but I'm also not opposed to having a better mechanism for the ordering of plugins.

In practice what this means is: I'm not sure either @tsloughter or myself would find the time to do this in our free time considering other things in the works (i.e. updating the hex API and dep handling). However, we'd probably be fine brainstorming ideas and guiding implementations if you (or anyone else) wishes to join the project and spend time on it.

@tothlac
Copy link
Contributor Author

tothlac commented Aug 22, 2018

Sure, I would be also interested in it.

In the meantime I've created a small pr which accidentally fixed all the issues I was struggling with excerpt the plugin ordering, but that's fine right now since alphabetical ordering worked fine. The build job on travis also looks fine. There was only one problem with it: #1859
Can you check this change?

@tothlac
Copy link
Contributor Author

tothlac commented Aug 22, 2018

Unfortunately it looks like even after the tests were not failing, and it solved the current problems there are still problems with this modification in even more complex cases.

@tothlac
Copy link
Contributor Author

tothlac commented Sep 10, 2018

Do you currently have any ideas how to fix it?
One more question: i saw your pkg handling changes on the hex_core branch. Do you also want to add pkg mirroring abilities to rebar3? What we need: create a local repository where we store all pkgs. If the pkg does not exist in the local repository try to get it from a remote repo, and push it automatically into the local repo + update the local registry.

@ferd
Copy link
Collaborator

ferd commented Sep 10, 2018

Hm, re-looking at the patch, the reason why you can't take the plugins out of the codepath they were is that dependency-handling plugins (custom resources) must be around during the compile phase for the .app.src file generation and version locking, which can be custom-made.

However I don't think there's a big negative to having the plugin path available earlier. Would your patch work if you added the path in both areas? I guess it would still remove the plugin from the path and that could be a bit of a problem?


regarding the new hex version, the branch is already usable (for testing and experimenting purposes). There is not a lot of doc, but if you pop into either the #rebar IRC channel on freenode or the #rebar3 channel on slack, Tristan can probably offer a bit of guidance on it.

The expected mechanism is to specify multiple indexes that will be checked in order for each dependency. There's some fancy logic with lock files to ensure only the right versions of some libs are fetched. There will be no automatic publication from one registry to the next one as part of fetching dependencies, simply because what people might want is not just mirrors, but also "overlay" repositories (i.e. I want to use my 5 corporate libraries from our private index with public libraries from hex). Auto-publishing into private environments could be super problematic for some setups.

I would probably instead create a kind of 'sync' job whose purpose was to read the remote index and simply mirror it.

@tothlac
Copy link
Contributor Author

tothlac commented Sep 13, 2018

I was finally able to add some changes to the code and with these changes everything works well. It is not a big modification, I just had to call code:add_pathsa(rebar_state:code_paths(State, all_plugin_deps)) from like 4 places. Everything works with it now, but of course rebar_plugins_SUITE fails when I run the tests. In my opinion all plugins should be loaded till execution ends. For example when during an upgrade rebar_utils:vcs_vsn_cmd/3 calls Module:make_vsn/1 the plugin should be still there, or when a command implemented by a plugin is called. Can you tell me more about why do you call rebar_utils:remove_from_code_paths/1 from some places? In which execution steps should be the plugin exactly removed from the code path and why?

@ferd
Copy link
Collaborator

ferd commented Sep 13, 2018

Say for example that you run a plugin that loads dependency B. Dependency B is on version 1.4.0 and has its own .hrl file.

You own application also has a dependency on B, but it specified B at version 1.5.0, which has different .hrl files that can be loaded in your app.

Since the compiler loads the .hrl files in include_lib based on currently loaded paths, the plugin will prevent the application from compiling against the proper dependency set. That's the reason why, just around the compiling phase, we unload the plugins. It ensures that the build will be repeatable based on what is in the lock file and not based on current plugins (which might be globally installed and not project-specific).

The same problem exists with parse_transforms as well, with a dependency such as lager with lager_transform.

@ferd
Copy link
Collaborator

ferd commented Apr 3, 2019

I believe this was fixed with all the code-handling patches in 3.7.0? Please reopen if this is still a problem

@ferd ferd closed this as completed Apr 3, 2019
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

No branches or pull requests

2 participants