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.

Sign up for GitHub

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

Have "rebar3 plugins upgrade" work without specifying plugin name #2521

Merged
merged 2 commits into from
Apr 2, 2021
Merged

Have "rebar3 plugins upgrade" work without specifying plugin name #2521

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de8bb90
Have "rebar3 plugins upgrade" keep its promise
paulo-ferraz-oliveira Mar 28, 2021
10404ab
Be more forgiving with input
paulo-ferraz-oliveira Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions src/rebar_prv_plugins.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

-export([init/1,
do/1,
format_error/1]).
format_error/1,
list_local_plugins/1]).

-include("rebar.hrl").
-include_lib("providers/include/providers.hrl").
Expand Down Expand Up @@ -41,22 +42,33 @@ do(State) ->

RebarOpts = rebar_state:opts(State),
SrcDirs = rebar_dir:src_dirs(RebarOpts, ["src"]),
Plugins = rebar_state:get(State, plugins, []),
ProjectPlugins = rebar_state:get(State, project_plugins, []),
{LocalPluginsDefs, _} = list_local_plugins(State),
PluginsDirs = filelib:wildcard(filename:join(rebar_dir:plugins_dir(State), "*")),

%% use `checkouts_dir' and not `checkouts_out_dir'. Since we use `all' in `find_apps'
%% so it doesn't need to be built and the apps in `checkouts_dir' could be old
%% because the user removing from `_checkouts/' doesn't cause removal of the output
CheckoutsDirs = filelib:wildcard(filename:join(rebar_dir:checkouts_dir(State), "*")),
Apps = rebar_app_discover:find_apps(CheckoutsDirs++PluginsDirs, SrcDirs, all, State),
display_plugins("Local plugins", Apps, Plugins ++ ProjectPlugins),
display_plugins("Local plugins", Apps, LocalPluginsDefs),
{ok, State}.

-spec format_error(any()) -> iolist().
format_error(Reason) ->
io_lib:format("~p", [Reason]).

list_local_plugins(State) ->
LocalPluginsDefs = rebar_state:get(State, plugins, [])
++ rebar_state:get(State, project_plugins, []),
LocalPluginsNames = lists:map(
fun (LocalPluginDef) ->
if is_atom(LocalPluginDef) -> LocalPluginDef;
is_tuple(LocalPluginDef) -> element(1, LocalPluginDef)
ferd marked this conversation as resolved.
Show resolved Hide resolved
end
end,
LocalPluginsDefs),
{LocalPluginsDefs, LocalPluginsNames}.

display_plugins(_Header, _Apps, []) ->
ok;
display_plugins(Header, Apps, Plugins) ->
Expand Down
10 changes: 7 additions & 3 deletions src/rebar_prv_plugins_upgrade.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,18 @@ do(State) ->
{Args, _} = rebar_state:command_parsed_args(State),
case proplists:get_value(plugin, Args, none) of
none ->
?PRV_ERROR(no_plugin_arg);
{_, LocalPluginsNames} = rebar_prv_plugins:list_local_plugins(State),
lists:foldl(
fun (LocalPluginName, {ok, StateAcc}) ->
upgrade(atom_to_list(LocalPluginName), StateAcc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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,

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, then I guess I might "bypass" those lines in my analysis, as you propose only looking at the 70-80 range.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

end,
{ok, State},
LocalPluginsNames);
Plugin ->
upgrade(Plugin, State)
end.

-spec format_error(any()) -> iolist().
format_error(no_plugin_arg) ->
io_lib:format("Must give an installed plugin to upgrade as an argument", []);
format_error({not_found, Plugin}) ->
io_lib:format("Plugin to upgrade not found: ~ts", [Plugin]);
format_error(Reason) ->
Expand Down