Have "rebar3 plugins upgrade" work without specifying plugin name #2521
Conversation
paulo-ferraz-oliveira
force-pushed
the
feature/rebar3_plugins_upgrade
branch
from
27c92d7 to
de8bb90
Compare
|
Assumptions:
|
| {_, LocalPluginsNames} = rebar_prv_plugins:list_local_plugins(State), | ||
| lists:foldl( | ||
| fun (LocalPluginName, {ok, StateAcc}) -> | ||
| upgrade(atom_to_list(LocalPluginName), StateAcc) |
There was a problem hiding this comment.
This is going to be rather slow or costly... you're calling the whole upgrade procedure for each plugin one at a time, including analysis. I can merge this since it's better than the current behaviour, but I'd look in seeing if we could improve this at some point by letting the code from lines 70 to 80 handle a larger set of applications in a list to build them at once.
It's not urgent since people upgrade plugins rather rarely, but the code is currently suboptimal.
There was a problem hiding this comment.
Sure, it's not urgent; and I will look into possibilities to improve it (this was mainly a first dirty approach to prove the concept). For context's sake: I've it plugged to CI, which means that I'm currently forced to know, beforehand, which plugins I want to upgrade and then do it. If it's in a generic Makefile, for example (whose consumers I know not what plugins chose), I could easily just call rebar3 plugins upgrade and let rebar3 take care of it. I'll change to draft to signal it's still ongoing.
There was a problem hiding this comment.
Fred, while looking at this I just saw I'm not taking profiles into account, but it seems to work OK (still didn't do the proposed changes); is the state updated previously to make sure the profiles are those from the input?
Otherwise, I don't understand
Profiles = rebar_state:current_profiles(State),
case find_plugin(Plugin, Profiles, State) of
not_found ->
Dep = find_plugin(Plugin, [global], State);
Dep ->
Dep
end,There was a problem hiding this comment.
the global profile is one magic thing that is super odd. Otherwise, the merging of state and handling of profiles should be transparent and nobody should need to care about them. It's why we aim at people not needing to know which profiles they run under.
There was a problem hiding this comment.
Cool, then I guess I might "bypass" those lines in my analysis, as you propose only looking at the 70-80 range.
There was a problem hiding this comment.
We shouldn't block this PR on these improvements, they're more of a follow-up. Having something working slow is better than something that doesn't work at all in this case.
There was a problem hiding this comment.
I'm ready to merge this whenever you're happy to take it out of draft mode.
Oh, cool. I'm OK to do that just now.
ferd
changed the title
Closes #2491.
As per the doc.s and a validation (over Slack), about the expected behaviour, this pull request aims at having
rebar3 plugins upgrade(no target plugin) work.as Profile plugins upgradeis taken into account (crashing in this pull request)