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

Remove duplicated message when trying to upgrade a checkout dep #2634

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove duplicated message when trying to upgrade a checkout dep #2634

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
d3f53b4
Remove duplicated message when trying to upgrade a checkout dep
pablocostass Oct 30, 2021
7bf61ce
Mark deps as checkouts, if needed, after calling the `lock` provider
pablocostass Oct 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
6 changes: 3 additions & 3 deletions src/rebar_prv_lock.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ do(State) ->
OldLockNames = [element(1,L) || L <- OldLocks] -- Checkouts,
NewLockNames = [element(1,L) || L <- Locks],

%% TODO: don't output this message if the dep is now a checkout
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already addresses by #2335, I think, but I forgot to remove it then.

rebar_utils:info_useless(OldLockNames, NewLockNames),
info_checkout_deps(Checkouts),

Expand All @@ -67,6 +66,7 @@ build_locks(State) ->
rebar_app_info:dep_level(Dep)}
end || Dep <- AllDeps, not(rebar_app_info:is_checkout(Dep))].

info_checkout_deps(Checkouts) ->
info_checkout_deps(Checkouts0) ->
Checkouts = lists:usort(Checkouts0),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes the checkouts list will have duplicated dep names, this should also help remove duplicated messages.

[?INFO("App ~ts is a checkout dependency and cannot be locked.", [CheckoutDep])
|| CheckoutDep <- Checkouts].
|| CheckoutDep <- Checkouts].
22 changes: 21 additions & 1 deletion src/rebar_prv_upgrade.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,32 @@ do_(State) ->
[element(1,Lock) || Lock <- Locks],
[rebar_app_info:name(App) || App <- rebar_state:lock(State5)]
),
rebar_prv_lock:do(State5);
State6 = maybe_ignore_checkouts(State5, Checkouts),
rebar_prv_lock:do(State6);
_ ->
Res
end
end.

-spec maybe_ignore_checkouts(rebar_state:t(), [binary()]) -> rebar_state:t().
maybe_ignore_checkouts(State, Checkouts) ->
OldLocks = rebar_state:get(State, {locks, default}, []),
OldLockNames = [element(1,L) || L <- OldLocks],
case OldLockNames -- Checkouts of
%% If there aren't any checkout dependencies in the old lock info,
%% we can safely "ignore" them to avoid a duplicated message from
%% `rebar_prv_lock:info_checkout_deps/1`.
OldLockNames ->
DepsIgnoringCheckout =
[case rebar_app_info:is_checkout(Dep) of
true -> rebar_app_info:is_checkout(Dep, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this will now hide status of whether a dep is a checkout dep to any provider that relies on locking (i.e. compiling). This may break a lot of stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought so too. If the state that lock uses has no checkout apps, it will definitely be passed onto other providers that lock calls/relies on.

Thanks for bringing it up, because for a second I forgot that although the code executed by lock will not have any issues, the provider returns its own state for others to use.

Would it make sense then to, in line 115, match on {ok, State7} = rebar_prv_lock:do(State6) and patch again the state by marking again as checkouts accordingly the dependencies?

Copy link
Collaborator

@ferd ferd Oct 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That might work. It could possibly be simpler to just keep a quick cache entry about it. The rebar agent does it internally by carrying explicit state:

rebar3/src/rebar_agent.erl

Lines 166 to 172 in c102379

%% @private function to display a warning for the feature only once
-spec maybe_show_warning(#state{}) -> #state{}.
maybe_show_warning(S=#state{show_warning=true}) ->
?WARN("This feature is experimental and may be modified or removed at any time.", []),
S#state{show_warning=false};
maybe_show_warning(State) ->
State.

The config handler uses the app env values to do it:

rebar3/src/rebar_config.erl

Lines 97 to 112 in ba50a08

%% @private outputs a warning for a newer lockfile format than supported
%% at most once.
%% The warning can also be cancelled by configuring the `warn_config_vsn'
%% OTP env variable.
-spec warn_vsn_once() -> ok.
warn_vsn_once() ->
Warn = application:get_env(rebar, warn_config_vsn) =/= {ok, false},
application:set_env(rebar, warn_config_vsn, false),
case Warn of
false -> ok;
true ->
?WARN("Rebar3 detected a lock file from a newer version. "
"It will be loaded in compatibility mode, but important "
"information may be missing or lost. It is recommended to "
"upgrade Rebar3.", [])
end.

I like that one because it also allows people to tweak or change the cache behaviour in weird cases like tests.

The pdict could work in more restrained cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah using app environment is a nice thing that rebar3 uses in some features. I do not think, however, it would translate well into removing a duplicated message :/

As for the cache, the idea is good, but given that the lock provider does not modify the list of dependencies, the simplest approach IMO is to keep said list in a variable and after calling that provider from upgrade, setting the list of dependencies of the new state to that. No need to introduce a record/cache to keep track of the list when the use case is this minimal, although I will keep it in mind for other providers :)

false -> Dep
end || Dep <- rebar_state:all_deps(State)],
rebar_state:all_deps(State, DepsIgnoringCheckout);
_ ->
State
end.

-spec format_error(any()) -> iolist().
format_error({unknown_dependency, Name}) ->
io_lib:format("Dependency ~ts not found", [Name]);
Expand Down