Major Providers Refactoring

[tsloughter]

I want to get all the current stakeholders involved in this discussion. @ferd @omarkj @talentdeficit @oubiwann @marianoguerra

In working on supporting subcommands for lfe and using erlydtl as my basis for it I'm thinking it is best supported by pushing more of the command and task handling into the providers app.

This would basically gut rebar_core, moving process_command and do to the providers module. It'll also require making a providers_state behaviour for rebar_state because functions to set state like command_args, command_parsed_args and apply_profiles are needed in process_command.

The main change to providers themselves would be init no longer returning {ok, State} but instead either {ok, providers:t() | list(), State} or {ok, providers:t() | list()}. Depending on if init should be able to update the state and if it should return a created provider or just the proplist that is used to create the provider.

[oubiwann]

This sounds like a good evolution of providers to me; my gut says that this work belongs with the providers.

[tsloughter]

Cool. So init could be cut down to:

init() ->
    [{name, ?PROVIDER},
     {module, ?MODULE},
     {bare, false},
     {deps, ?DEPS},
     {example, "rebar erlydtl <subtask>"},
     {short_desc, ""},
     {desc, ""},
     {subproviders, [rebar_prv_erlydtl_compiler]},
     {opts, []}].

or we could move the definition of the provider record to providers.hrl and have init return the record. But I don't really like not wrapping records in a module.

[tsloughter]

And could get rid of the need for modifying State in the providers module by changing do/1 to do(RawArgs, ParsedArgs, State). Hm...

[ferd]

Here's an alternative approach. Providers can define an optional namespace they operate in. You have a single provider that behaves like do, but it's a generic one with a hidden name.

Default rebar stuff would put in nothing (and would be wrapped by do). LFE stuff would work within a lfe namespace, elixir in the ex namespace for example.

So when I call rebar3 lfe compile and there is no provider attached to the lfe name in the default namespace, we look up the lfe namespace, and check for the compile provider (letting you have compile in many namespaces!) and run that one instead.

This requires little to no change in provider support (just new options), and allows a nearly unlimited amount of namespaces for custom projects and languages without requiring central coordination from us.

What do you think?

[ferd]

Oh and a sweet thing this could enable is a python-like 'future' namespace. Want to try a new compile option for default rebar3 in Erlang? rebar3 future compile and it loads a different compile option that people can use and try.

[tsloughter]

That could work. Will need a way of defining namespaces such that someone can include the plugin lfe and it's namespace and providers under that namespace get loaded.

[ferd]

To be more precise, (and I'll try to build a demo test thing ASAP):
in https://github.com/rebar/rebar3/blob/master/src/rebar_core.erl#L39-L46, first we look at the command and try to find it in the default namespace. If it's not found, we use the command as a namespace identifier.

do maps to default, lfe maps to lfe, ex maps to ex, etc. so that plugins can define their namespace and it automatically creates the equivalent do command for them.

https://github.com/tsloughter/providers/blob/master/src/providers.erl#L29 would probably need a namespace option there
https://github.com/tsloughter/providers/blob/master/src/providers.erl#L166-L167 a version of this function could also use a namespace when matching, or have the current one redirect to a default search.

With this stuff in place, we could probably extend the current do to work and take many aliases as plugins decide to define them.

If I have time tomorrow, I'll try to get a proof of concept going.

[oubiwann]

Oh, wow. This is the most idiomatic Erlang suggestion I've heard so far.

Questions:

• How amenable would this be to multiple namespace support? (I'm guessing we'd just have to make the namespace a list instead of an atom ...).
• This would allow LFE, for example, to use an Elixir test runner when calling rebar3 lfe tests (as long as the Elixir provider registered itself for both lfe and ex namespaces).
• Hrm, what about preventing a provider from running in a namespace? If Joxa declares a test runner that works for Elixir and LFE, but LFE wants to use a different test runner ...
• A provider could also register itself for multiple tasks, yes? So rebar3 lfe compile and rebar3 lfe tests are registered by a provider ... but what if one is good, and the community likes it, but others don't like the second (e.g., the tests) provider. How would one prevent over-zealous projects from grabbing tasks in a namespace?

Also: bonus points for minimizing the changes :-)

[ferd]

In my own view, a provider is registered to a single namespace, but namespace creation is dynamic. As soon as a provider declares it belongs to a given namespace, that namespace is created.

I think trying to reuse the same provider for multiple namespaces makes madness lie this way because dependency chains and sequences of calls will not be the same, and debugging it will probably be a nightmare.

I don't think this is flexibility that is needed nearly as much as initial laziness (I'll just reuse this pile of code blindly). If code needs to be shared, extract it to a third-party library and then declare independent providers that depend on it. Again, this is my view so far.

With that simple mapping, your second, third, and fourth questions are now a non-issue. If you don't want a provider, don't install it as a plugin. Problem solved.

[ferd]

There's also gonna be the tricky way of how to declare dependencies on steps that ran in a different or local namespace. Currently we use Name and {Profile, Name} to specify dependencies. The new form might force us to consider Name, {Profile, Name}, and {Namespace, Profile, Name}, where Name expands to {default, Name}, and {Profile, Name} expands to {CurrentNamespace, Profile, Name}.

[tsloughter]

Actually, we can get rid of {Profile, Name}. After we switching to using a list of profiles that combine into a single State it is no longer needed or used. I simply haven't removed it from the providers app yet.

[tsloughter]

So it would be just {Namespace, Name} if we need it.

[ferd]

Cool, that's even nicer then.

[oubiwann]

Definitely agree with the therein-lies-madness. I started having flashbacks
to graph-traversals to calculate dependency chains from code 10 years ago.
Would gladly support a party line of "we're not going down that path in
order to protect you from yourself" :-)

On Sat, Dec 20, 2014 at 9:44 PM, Fred Hebert notifications@github.com
wrote:

In my own view, a provider is registered to a single namespace, but
namespace creation is dynamic. As soon as a provider declares the
namespace, it is created.

I think trying to reuse the same provider for multiple namespaces makes
madness lie this way because dependency chains and sequences of calls will
not be the same, and debugging it will probably be a nightmare.

I don't think this is flexibility that is needed nearly as much as
initial laziness (I'll just reuse this pile of code blindly). If code needs
to be shared, extract it to a third-party library and then declare
independent providers that depend on it. Again, this is my view so far.

With that simple mapping, your second, third, and fourth questions are now
a non-issue. If you don't want a provider, don't install it as a plugin.
Problem solved.

—
Reply to this email directly or view it on GitHub
#69 (comment).

[talentdeficit]

what about the currently valid rebar clean compile test? does that just not work anymore?

also what does just rebar ex or rebar lfe do? (they should probably do rebar ex help and rebar lfe help respectively)

[marianoguerra]

I think by default a command with subcommands should register the default command to run when none given, just like make does.

one question, how to specify commands with subcommands inside "meta commands"

something like

rebar do (lfe compile) (lfe test)

or

rebar do lfe:compile lfe:test

[ferd]

@talentdeficit this is currently supported by do and would remain so. rebar3 do clean compile test.

@marianoguerra my proposal would make lfe act like do within the lfe namespace. Therefore rebar3 lfe compile test would work the same for lfe as rebar3 do compile test does for regular rebar3 Erlang providers.

[tsloughter]

It is rebar3 do clean, compile, test

The commas are necessary because each task has its own arguments. Which does need a fix because right now it simply uses string:tokens and breaks if an arg takes a string like, --desc "some, strong".

[ferd]

Demo ready in #70

[talentdeficit]

what about a mix like syntax for subproviders? then the following would all be valid:

$ rebar clean compile eunit
$ rebar compile ct --suite rebar_state_SUITE, rebar_core_SUITE, rebar_prv_common_test_SUITE clean
$ rebar lfe.compile lfe.test
$ rebar lfe
$ rebar ex.compile --jobs 10 ex.test --seed "entropy" hex.publish

[ferd]

why do you want to compile before running eunit? You don't and shouldn't need to anymore, ever. This was always a terribly shitty experience of rebar IMO. Commands know their dependencies.

Not sure what the current format for the second command is, but this should be supported with 'do'.

For the fourth one, this would be 'rebar3 lfe compile test' under my proposal.

The fifth form (mixing namespaces in a single run) wouldn't be supported under my proposal, but would be rebar3 ex compile --jobs 10, test --seed "entropy" && rebar3 hex publish I believe? (@tsloughter can correct me), but one could create a set of 'workflow' providers that can manually order cross-namespaces in their deps.

[tsloughter]

@talentdeficit the issue is it doesn't make clear what args go with what command if you don't use commas between the commands. You could say that any args for compile has to come before ct but that complicates parsing and is unclear to a user.

As for use of . I just don't like it :)

[talentdeficit]

i'm ok with either syntax, i was just offering an alternate that would not be a breaking change

[tsloughter]

@ferd it would be rebar3 lfe compile, test, right?

[talentdeficit]

i did a github search and there's way less instances of rebar foo bar than i expected so breaking backwards compatibility in this case is probably a nonissue

[ferd]

@tsloughter right.

@talentdeficit thanks for the search, that's good to know.

[tsloughter]

Namespaces have been merged in and updated the erlydtl provider for it https://github.com/rebar/rebar3/blob/master/src/rebar_prv_erlydtl_compiler.erl

I think now we need a good way to include a multiple providers via a plugin. Right now it is 1:1 for plugins and providers. We'll want the user to be able to specify the lfe plugin and all of its providers get registered into rebar3.

[ferd]

Ah yeah. I'm guessing that's different from a multi-provider like having them submit their own do, but would be more about having a quick way to represent a namespace's entire set of plugins as one single dependency?

[tsloughter]

Yea, it may just be a function each plugin has to export so rebar3 can saying Plugin:providers() and get a list of the provider modules to init.

[ferd]

I'm saying the problem would be on the plugin download itself, not the rebar3 download. I added a config file by hand before applying the template and used the https:// version of the plugin URL and it worked first try for me.

[tsloughter]

Weird, changing the config does not work for me with the downloaded rebar3. Only works with my local rebar3 built from master.

[marianoguerra]

yes, I'm downloading rebar3 with:

wget https://s3.amazonaws.com/rebar3/rebar3

should I try building it from source?

[marianoguerra]

yep, building from sources work.

maybe the nightly's job isn't running or something like that?

[marianoguerra]

hi, now I'm at it again trying to make a efene ct plugin whose only task is to compile efene code on test/ and then delegate to rebar3's own ct task.

is there a way to call another task explicitly from a plugin at a point that is not before? (that would be solved by putting the other task in the dependencies of this task).

[tsloughter]

We'll need tests to make sure this is still working and doesn't break, but there is an element of provider called hooks that takes a tuple of 2 lists, by default: {[], []}. The first list is providers to run before and the second is to run after. So you should be able to add to your provider list of attributes: {hooks, {[], [ct]}}

[marianoguerra]

should I namespace it with default in my case that I'm using another namespace?

[tsloughter]

Yea

[marianoguerra]

I think you should "resolve" tuples to provider records in this line for it to work?

https://github.com/tsloughter/providers/blob/master/src/providers.erl#L86

given how it's called here:

https://github.com/tsloughter/providers/blob/master/src/providers.erl#L93

[marianoguerra]

yep, getting the provider like this on my plugin and adding DefaultCTProvider to the post hooks works

  Providers = rebar_state:providers(State),                                  
  DefaultCTProvider = providers:get_provider({default, ct}, Providers),

but I think that should be done on the link provided above (or on rebar3 since providers are in state and afaik providers module doesn't know about rebar_state)

[tsloughter]

Ah yup, I'll patch that soon.

[tsloughter]

@marianoguerra get_provider should work with providers:get_provider(ct, Providers) just fine. That is what you were asking for, right? https://github.com/tsloughter/providers/blob/master/src/providers.erl#L204

I've also got a patch I'll get out today that replaces profile with profiles in providers.

[tsloughter]

Can see the profiles change #96

[marianoguerra]

@tsloughter you mean I shoud do

{hooks, {[], [providers:get_provider({default, ct}, Providers)]}}

instead of

{hooks, {[], [{default, ct}]}}

on my code or that should be done on rebar's side?

[tsloughter]

Oh, it isn't resolving them but instead expects the hook list to be of the actual Provider record? That should change if that is the case. I look at that.

[marianoguerra]

@tsloughter yes, I think that's happening, see the links here #69 (comment)

[tsloughter]

So... this turned out to be a pain.

To get a provider from the name it needs the list of providers. While this is contained in the State variable we can't access that from providers since it is rebar_state and we can't call to rebar from providers.

So neither do/2 or run_all/2 can resolve the atom to a provider...

Not sure how to handle this yet...

[ferd]

Couldn't rebar_hooks take care of expanding that stuff? I'm guessing ideally you wanted to resolve them whenever the provider is loaded in memory or something?

The best option would be to internally expand them when calling providers:get_target_providers(...) If we fear people may use mixed forms, we'd need an internal flag to note when hooks have been 'normalized' or something, or always do it dynamically.

This is not particularly different from being able to define deps of a provider by its {Namespace, Name} combo, so fixing it in providers directly would probably be good for the library's consistency.

(edited to change the reference for a function)

[tsloughter]

rebar_hooks doesn't use providers.

We can only use get_target_providers if the list of providers is available. If you look at create and do in providers you'll see that that isn't available. It "is" in the sense that it is in State but we can't call rebar_state:providers from providers.

[ferd]

I thought get_target_providers is how we could constrain the list of providers we have to run one we know about. It gets a list of providers passed to it, and it should be possible to have a pass over the list of all providers.

Say after process_deps is being called.

get_target_providers(Target, Providers, Namespace) ->
    TargetProviders = lists:filter(fun(#provider{name=T, namespace=NS})
                                         when T =:= Target, NS =:= Namespace ->
                                           true;
                                      (_) ->
                                           false
                                   end, Providers),
    expand_hooks(process_deps(TargetProviders, Providers), Providers).

I'm not exactly sure what can't work there?

[tsloughter]

Oh, that will work. It does mean that a provider that has only been created but not going through expand_hooks will fail to run its hooks though. But maybe that is an acceptable restriction for providers to have.

[tsloughter]

Ok, mostly got this working. But a new question. Resolving the deps of the hook provider?

Example: We have provider A with hook = {[B], []} and provider B has deps = [C]. Does a run of A become: [C, B, A] or just [B, A], I assume the former. If correct then the complications come if A also defines deps = [C]. Does this mean a single run of C? Or 2? Currently if A has B as a hook and a dep it will run B twice, so that would lead me to say C is run twice.

Thoughts?

[ferd]

I'd probably go for [B,A] at first and readvise if there's ever a need for more. If A depends on C and has a pre-hook B, then it should be [C,B,A] and we at least have a way to workaround hooks issues if it ever comes to that by just adding them to A.

[tsloughter]

I find that weird because it is basically saying that using a provider as a hook is only supported if the provider doesn't rely on any deps.

[ferd]

Yes. I'm assuming the provider in the hook would have similar requirements to the currently running one, otherwise why call for the hook? Do we expect some kind of do_everything provider that calls to ct, compile, lock, and then new just because?

I'm thinking that if you're writing a pre-processor of some kind, you may depend on install_deps and want to have compile run after you.

I'm just trying hard to think of a good use case where someone goes "I have this here plugin that does something and then calls release but didn't bother compiling files or installing deps first" and I kind of come short on it.

[tsloughter]

Does this issue need to stay open?

[marianoguerra]

I need to update my plugin to the latest version and check if everything works ok, you can close it and I can reopen it or open a new one if you want.