Merge pull request #1070 from project-fifo/hex-improvements

Hex improvements

src/rebar_packages.erl

@@ -7,7 +7,9 @@
         ,registry_dir/1
         ,package_dir/1
         ,registry_checksum/2
+        ,find_highest_matching/6
         ,find_highest_matching/4
+        ,find_all/3
         ,verify_table/1
         ,format_error/1]).
 
@@ -139,16 +141,26 @@ registry_checksum({pkg, Name, Vsn}, State) ->
 %% `~> 2.0` | `>= 2.0.0 and < 3.0.0`
 %% `~> 2.1` | `>= 2.1.0 and < 3.0.0`
 find_highest_matching(Dep, Constraint, Table, State) ->
+    find_highest_matching(undefined, undefined, Dep, Constraint, Table, State).
+
+find_highest_matching(Pkg, PkgVsn, Dep, Constraint, Table, State) ->
+    try find_all(Dep, Table, State) of
+        {ok, [Vsn]} ->
+            handle_single_vsn(Pkg, PkgVsn, Dep, Vsn, Constraint);
+        {ok, [HeadVsn | VsnTail]} ->
+                            {ok, handle_vsns(Constraint, HeadVsn, VsnTail)}
+    catch
+        error:badarg ->
+            none
+    end.
+
+find_all(Dep, Table, State) ->
     ?MODULE:verify_table(State),
     try ets:lookup_element(Table, Dep, 2) of
-        [[HeadVsn | VsnTail]] ->
-            {ok, handle_vsns(Constraint, HeadVsn, VsnTail)};
-        [[Vsn]] ->
-            handle_single_vsn(Dep, Vsn, Constraint);
-        [Vsn] ->
-            handle_single_vsn(Dep, Vsn, Constraint);
-        [HeadVsn | VsnTail] ->
-            {ok, handle_vsns(Constraint, HeadVsn, VsnTail)}
+        [Vsns] when is_list(Vsns)->
+            {ok, Vsns};
+        Vsns ->
+            {ok, Vsns}
     catch
         error:badarg ->
             none
@@ -165,13 +177,19 @@ handle_vsns(Constraint, HeadVsn, VsnTail) ->
                         end
                 end, HeadVsn, VsnTail).
 
-handle_single_vsn(Dep, Vsn, Constraint) ->
+handle_single_vsn(Pkg, PkgVsn, Dep, Vsn, Constraint) ->
     case ec_semver:pes(Vsn, Constraint) of
         true ->
             {ok, Vsn};
         false ->
-            ?WARN("Only existing version of ~s is ~s which does not match constraint ~~> ~s. "
-                 "Using anyway, but it is not guaranteed to work.", [Dep, Vsn, Constraint]),
+            case {Pkg, PkgVsn} of
+                {undefined, undefined} ->
+                    ?WARN("Only existing version of ~s is ~s which does not match constraint ~~> ~s. "
+                          "Using anyway, but it is not guaranteed to work.", [Dep, Vsn, Constraint]);
+                _ ->
+                    ?WARN("[~s:~s] Only existing version of ~s is ~s which does not match constraint ~~> ~s. "
+                          "Using anyway, but it is not guaranteed to work.", [Pkg, PkgVsn, Dep, Vsn, Constraint])
+            end,
             {ok, Vsn}
     end.
 

src/rebar_prv_update.erl

@@ -11,6 +11,10 @@
 
 -export([hex_to_index/1]).
 
+-ifdef(TEST).
+-export([cmp_/6, cmpl_/6, valid_vsn/1]).
+-endif.
+
 -include("rebar.hrl").
 -include_lib("providers/include/providers.hrl").
 
@@ -99,7 +103,7 @@ hex_to_index(State) ->
                 ets:foldl(fun({{Pkg, PkgVsn}, [Deps, Checksum, BuildTools | _]}, _) when is_list(BuildTools) ->
                                   case lists:any(fun is_supported/1, BuildTools) of
                                       true ->
-                                          DepsList = update_deps_list(Deps, Registry, State),
+                                          DepsList = update_deps_list(Pkg, PkgVsn, Deps, Registry, State),
                                           ets:insert(?PACKAGE_TABLE, {{Pkg, PkgVsn}, DepsList, Checksum});
                                       false ->
                                           true
@@ -137,20 +141,114 @@ hex_to_index(State) ->
             fail
     end.
 
-update_deps_list(Deps, HexRegistry, State) ->
+update_deps_list(Pkg, PkgVsn, Deps, HexRegistry, State) ->
     lists:foldl(fun([Dep, DepVsn, false, _AppName | _], DepsListAcc) ->
-                        case DepVsn of
-                            <<"~> ", Vsn/binary>> ->
-                                case rebar_packages:find_highest_matching(Dep, Vsn, HexRegistry, State) of
-                                    {ok, HighestDepVsn} ->
-                                        [{Dep, HighestDepVsn} | DepsListAcc];
-                                    none ->
-                                        ?WARN("Missing registry entry for package ~s. Try to fix with `rebar3 update`", [Dep]),
-                                        DepsListAcc
-                                end;
-                            Vsn ->
+                        Dep1 = {Pkg, PkgVsn, Dep},
+                        case {valid_vsn(DepVsn), DepVsn} of
+                            %% Those are all not perfectly implemented!
+                            %% and doubled since spaces seem not to be
+                            %% enforced
+                            {false, Vsn} ->
+                                ?WARN("[~s:~s], Bad dependency version for ~s: ~s.",
+                                      [Pkg, PkgVsn, Dep, Vsn]),
+                                DepsListAcc;
+                            {_, <<"~>", Vsn/binary>>} ->
+                                highest_matching(Dep1, rm_ws(Vsn), HexRegistry,
+                                                 State, DepsListAcc);
+                            {_, <<">=", Vsn/binary>>} ->
+                                cmp(Dep1, rm_ws(Vsn), HexRegistry, State,
+                                    DepsListAcc, fun ec_semver:gte/2);
+                            {_, <<">", Vsn/binary>>} ->
+                                cmp(Dep1, rm_ws(Vsn), HexRegistry, State,
+                                    DepsListAcc, fun ec_semver:gt/2);
+                            {_, <<"<=", Vsn/binary>>} ->
+                                cmpl(Dep1, rm_ws(Vsn), HexRegistry, State,
+                                     DepsListAcc, fun ec_semver:lte/2);
+                            {_, <<"<", Vsn/binary>>} ->
+                                cmpl(Dep1, rm_ws(Vsn), HexRegistry, State,
+                                     DepsListAcc, fun ec_semver:lt/2);
+                            {_, <<"==", Vsn/binary>>} ->
+                                [{Dep, Vsn} | DepsListAcc];
+                            {_, Vsn} ->
                                 [{Dep, Vsn} | DepsListAcc]
                         end;
                    ([_Dep, _DepVsn, true, _AppName | _], DepsListAcc) ->
                         DepsListAcc
                 end, [], Deps).
+
+rm_ws(<<" ", R/binary>>) ->
+    rm_ws(R);
+rm_ws(R) ->
+    R.
+
+valid_vsn(Vsn) ->
+    %% Regepx from https://github.com/sindresorhus/semver-regex/blob/master/index.js
+    SemVerRegExp = "v?(0|[1-9][0-9]*)\\.(0|[1-9][0-9]*)(\\.(0|[1-9][0-9]*))?"
+        "(-[0-9a-z-]+(\\.[0-9a-z-]+)*)?(\\+[0-9a-z-]+(\\.[0-9a-z-]+)*)?",
+    SupportedVersions = "^(>=?|<=?|~>|==)?\\s*" ++ SemVerRegExp ++ "$",
+    re:run(Vsn, SupportedVersions) =/= nomatch.
+
+highest_matching({Pkg, PkgVsn, Dep}, Vsn, HexRegistry, State, DepsListAcc) ->
+    case rebar_packages:find_highest_matching(Pkg, PkgVsn, Dep, Vsn, HexRegistry, State) of
+        {ok, HighestDepVsn} ->
+            [{Dep, HighestDepVsn} | DepsListAcc];
+        none ->
+            ?WARN("[~s:~s] Missing registry entry for package ~s. Try to fix with `rebar3 update`",
+                  [Pkg, PkgVsn, Dep]),
+            DepsListAcc
+    end.
+
+cmp({_Pkg, _PkgVsn, Dep} = Dep1, Vsn, HexRegistry, State, DepsListAcc, CmpFun) ->
+    {ok, Vsns}  = rebar_packages:find_all(Dep, HexRegistry, State),
+    cmp_(undefined, Vsn, Vsns, DepsListAcc, Dep1, CmpFun).
+
+
+cmp_(undefined, _MinVsn, [], DepsListAcc, {Pkg, PkgVsn, Dep}, _CmpFun) ->
+    ?WARN("[~s:~s] Missing registry entry for package ~s. Try to fix with `rebar3 update`",
+          [Pkg, PkgVsn, Dep]),
+    DepsListAcc;
+cmp_(HighestDepVsn, _MinVsn, [], DepsListAcc, {_Pkg, _PkgVsn, Dep}, _CmpFun) ->
+    [{Dep, HighestDepVsn} | DepsListAcc];
+
+cmp_(BestMatch, MinVsn, [Vsn | R], DepsListAcc, Dep, CmpFun) ->
+    case CmpFun(Vsn, MinVsn) of
+        true ->
+            cmp_(Vsn, Vsn, R, DepsListAcc, Dep, CmpFun);
+        false  ->
+            cmp_(BestMatch, MinVsn, R, DepsListAcc, Dep, CmpFun)
+    end.
+
+%% We need to treat this differently since we want a version that is LOWER but
+%% the higest possible one.
+cmpl({_Pkg, _PkgVsn, Dep} = Dep1, Vsn, HexRegistry, State, DepsListAcc, CmpFun) ->
+    {ok, Vsns}  = rebar_packages:find_all(Dep, HexRegistry, State),
+    cmpl_(undefined, Vsn, Vsns, DepsListAcc, Dep1, CmpFun).
+
+cmpl_(undefined, _MaxVsn, [], DepsListAcc, {Pkg, PkgVsn, Dep}, _CmpFun) ->
+    ?WARN("[~s:~s] Missing registry entry for package ~s. Try to fix with `rebar3 update`",
+          [Pkg, PkgVsn, Dep]),
+    DepsListAcc;
+
+cmpl_(HighestDepVsn, _MaxVsn, [], DepsListAcc, {_Pkg, _PkgVsn, Dep}, _CmpFun) ->
+    [{Dep, HighestDepVsn} | DepsListAcc];
+
+cmpl_(undefined, MaxVsn, [Vsn | R], DepsListAcc, Dep, CmpFun) ->
+    case CmpFun(Vsn, MaxVsn) of
+        true ->
+            cmpl_(Vsn, MaxVsn, R, DepsListAcc, Dep, CmpFun);
+        false  ->
+            cmpl_(undefined, MaxVsn, R, DepsListAcc, Dep, CmpFun)
+    end;
+
+cmpl_(BestMatch, MaxVsn, [Vsn | R], DepsListAcc, Dep, CmpFun) ->
+    case CmpFun(Vsn, MaxVsn) of
+        true ->
+            case ec_semver:gte(Vsn, BestMatch) of
+                true ->
+                    cmpl_(Vsn, MaxVsn, R, DepsListAcc, Dep, CmpFun);
+                false ->
+                    cmpl_(BestMatch, MaxVsn, R, DepsListAcc, Dep, CmpFun)
+            end;
+        false  ->
+            cmpl_(BestMatch, MaxVsn, R, DepsListAcc, Dep, CmpFun)
+    end.

test/rebar_deps_SUITE.erl

@@ -3,7 +3,7 @@
 -include_lib("common_test/include/ct.hrl").
 -include_lib("eunit/include/eunit.hrl").
 
-all() -> [sub_app_deps, newly_added_dep, newly_added_after_empty_lock, http_proxy_settings, https_proxy_settings, {group, git}, {group, pkg}].
+all() -> [sub_app_deps, newly_added_dep, newly_added_after_empty_lock, http_proxy_settings, https_proxy_settings, semver_matching_lt, semver_matching_lte, semver_matching_gt, valid_version, {group, git}, {group, pkg}].
 
 groups() ->
     [{all, [], [flat, pick_highest_left, pick_highest_right,
@@ -29,6 +29,14 @@ init_per_group(_, Config) ->
 end_per_group(_, Config) ->
     Config.
 
+init_per_testcase(valid_version, Config) ->
+    rebar_test_utils:init_rebar_state(Config);
+init_per_testcase(semver_matching_lt, Config) ->
+    rebar_test_utils:init_rebar_state(Config);
+init_per_testcase(semver_matching_lte, Config) ->
+    rebar_test_utils:init_rebar_state(Config);
+init_per_testcase(semver_matching_gt, Config) ->
+    rebar_test_utils:init_rebar_state(Config);
 init_per_testcase(newly_added_after_empty_lock, Config) ->
     rebar_test_utils:init_rebar_state(Config);
 init_per_testcase(newly_added_dep, Config) ->
@@ -49,14 +57,14 @@ init_per_testcase(http_proxy_settings, Config) ->
     %% Insert proxy variables into config
     rebar_test_utils:create_config(GlobalConfigDir,
                                    [{http_proxy, "http://localhost:1234"}
-    ]),
+                                   ]),
     rebar_test_utils:init_rebar_state(Config);
 init_per_testcase(https_proxy_settings, Config) ->
     SupportsHttpsProxy = case erlang:system_info(otp_release) of
-        "R16"++_ -> true;
-        "R"++_ -> false;
-        _ -> true % 17 and up don't have a "R" in the version
-    end,
+                             "R16"++_ -> true;
+                             "R"++_ -> false;
+                             _ -> true % 17 and up don't have a "R" in the version
+                         end,
     if not SupportsHttpsProxy ->
             {skip, https_proxy_unsupported_before_R16};
        SupportsHttpsProxy ->
@@ -73,20 +81,20 @@ init_per_testcase(https_proxy_settings, Config) ->
             %% Insert proxy variables into config
             rebar_test_utils:create_config(GlobalConfigDir,
                                            [{https_proxy, "http://localhost:1234"}
-            ]),
+                                           ]),
             rebar_test_utils:init_rebar_state(Config)
     end;
 init_per_testcase(Case, Config) ->
     {Deps, Warnings, Expect} = deps(Case),
     Expected = case Expect of
-        {ok, List} -> {ok, format_expected_deps(List)};
-        {error, Reason} -> {error, Reason}
-    end,
+                   {ok, List} -> {ok, format_expected_deps(List)};
+                   {error, Reason} -> {error, Reason}
+               end,
     DepsType = ?config(deps_type, Config),
     mock_warnings(),
     [{expect, Expected},
      {warnings, Warnings}
-    | setup_project(Case, Config, rebar_test_utils:expand_deps(DepsType, Deps))].
+     | setup_project(Case, Config, rebar_test_utils:expand_deps(DepsType, Deps))].
 
 end_per_testcase(https_proxy_settings, Config) ->
     meck:unload(rebar_dir),
@@ -100,8 +108,8 @@ end_per_testcase(_, Config) ->
 
 format_expected_deps(Deps) ->
     [case Dep of
-        {N,V} -> {dep, N, V};
-        N -> {dep, N}
+         {N,V} -> {dep, N, V};
+         N -> {dep, N}
      end || Dep <- Deps].
 
 %% format:
@@ -208,7 +216,7 @@ sub_app_deps(Config) ->
 
     SubAppsDir = filename:join([AppDir, "apps", Name]),
     SubDeps = rebar_test_utils:top_level_deps(rebar_test_utils:expand_deps(git, [{"a", "1.0.0", []}
-                                              ,{"b", "2.0.0", []}])),
+                                                                                ,{"b", "2.0.0", []}])),
     rebar_test_utils:create_app(SubAppsDir, Name, Vsn, [kernel, stdlib]),
     rebar_test_utils:create_config(SubAppsDir, [{deps, SubDeps}]),
 
@@ -242,12 +250,12 @@ newly_added_dep(Config) ->
 
     %% Add a and c to top level
     TopDeps2 = rebar_test_utils:top_level_deps(rebar_test_utils:expand_deps(git, [{"a", "1.0.0", []}
-                                               ,{"c", "2.0.0", []}
-                                               ,{"b", "1.0.0", []}])),
+                                                                                 ,{"c", "2.0.0", []}
+                                                                                 ,{"b", "1.0.0", []}])),
     {ok, RebarConfig2} = file:consult(rebar_test_utils:create_config(AppDir, [{deps, TopDeps2}])),
     LockFile = filename:join(AppDir, "rebar.lock"),
     RebarConfig3 = rebar_config:merge_locks(RebarConfig2,
-                                           rebar_config:consult_lock_file(LockFile)),
+                                            rebar_config:consult_lock_file(LockFile)),
 
     %% a should now be installed and c should not change
     rebar_test_utils:run_and_check(
@@ -277,7 +285,7 @@ newly_added_after_empty_lock(Config) ->
     {ok, RebarConfig2} = file:consult(rebar_test_utils:create_config(AppDir, [{deps, TopDeps2}])),
     LockFile = filename:join(AppDir, "rebar.lock"),
     RebarConfig3 = rebar_config:merge_locks(RebarConfig2,
-                                           rebar_config:consult_lock_file(LockFile)),
+                                            rebar_config:consult_lock_file(LockFile)),
 
     %% a should now be installed and c should not change
     rebar_test_utils:run_and_check(
@@ -304,6 +312,74 @@ https_proxy_settings(_Config) ->
                  httpc:get_option(https_proxy, rebar)).
 
 
+semver_matching_lt(_Config) ->
+    Dep = <<"test">>,
+    Dep1 = {Dep, <<"1.0.0">>, Dep},
+    MaxVsn = <<"0.2.0">>,
+    Vsns = [<<"0.1.7">>, <<"0.1.9">>, <<"0.1.8">>, <<"0.2.0">>, <<"0.2.1">>],
+    ?assertEqual([{Dep, <<"0.1.9">>}],
+                 rebar_prv_update:cmpl_(undefined, MaxVsn, Vsns, [], Dep1,
+                                        fun ec_semver:lt/2)).
+
+semver_matching_lte(_Config) ->
+    Dep = <<"test">>,
+    Dep1 = {Dep, <<"1.0.0">>, Dep},
+    MaxVsn = <<"0.2.0">>,
+    Vsns = [<<"0.1.7">>, <<"0.1.9">>, <<"0.1.8">>, <<"0.2.0">>, <<"0.2.1">>],
+    ?assertEqual([{Dep, <<"0.2.0">>}],
+                 rebar_prv_update:cmpl_(undefined, MaxVsn, Vsns, [], Dep1,
+                                        fun ec_semver:lte/2)).
+
+semver_matching_gt(_Config) ->
+    Dep = <<"test">>,
+    Dep1 = {Dep, <<"1.0.0">>, Dep},
+    MaxVsn = <<"0.2.0">>,
+    Vsns = [<<"0.1.7">>, <<"0.1.9">>, <<"0.1.8">>, <<"0.2.0">>, <<"0.2.1">>],
+    ?assertEqual([{Dep, <<"0.2.1">>}],
+                 rebar_prv_update:cmp_(undefined, MaxVsn, Vsns, [], Dep1,
+                                       fun ec_semver:gt/2)).
+semver_matching_gte(_Config) ->
+    Dep = <<"test">>,
+    Dep1 = {Dep, <<"1.0.0">>, Dep},
+    MaxVsn = <<"0.2.0">>,
+    Vsns = [<<"0.1.7">>, <<"0.1.9">>, <<"0.1.8">>, <<"0.2.0">>],
+    ?assertEqual([{Dep, <<"0.2.0">>}],
+                 rebar_prv_update:cmp_(undefined, MaxVsn, Vsns, [], Dep1,
+                                       fun ec_semver:gt/2)).
+
+valid_version(_Config) ->
+    ?assert(rebar_prv_update:valid_vsn(<<"0.1">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<" 0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"  0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"<0.1">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"<0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"< 0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"<  0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<">0.1">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<">0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"> 0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<">  0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"<=0.1">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"<=0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"<= 0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"<=  0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<">=0.1">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<">=0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<">= 0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<">=  0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"==0.1">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"==0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"== 0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"==  0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"~>0.1">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"~>0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"~> 0.1.0">>)),
+    ?assert(rebar_prv_update:valid_vsn(<<"~>  0.1.0">>)),
+    ?assertNot(rebar_prv_update:valid_vsn(<<"> 0.1.0 and < 0.2.0">>)),
+    ok.
+
+
 run(Config) ->
     {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)),
     rebar_test_utils:run_and_check(

test/rebar_pkg_SUITE.erl

@@ -180,11 +180,14 @@ pkgs_provider(Config) ->
 
 find_highest_matching(_Config) ->
     State = rebar_state:new(),
-    {ok, Vsn} = rebar_packages:find_highest_matching(<<"goodpkg">>, <<"1.0.0">>, package_index, State),
+    {ok, Vsn} = rebar_packages:find_highest_matching(
+                  <<"test">>, <<"1.0.0">>, <<"goodpkg">>, <<"1.0.0">>, package_index, State),
     ?assertEqual(<<"1.0.1">>, Vsn),
-    {ok, Vsn1} = rebar_packages:find_highest_matching(<<"goodpkg">>, <<"1.0">>, package_index, State),
+    {ok, Vsn1} = rebar_packages:find_highest_matching(
+                   <<"test">>, <<"1.0.0">>, <<"goodpkg">>, <<"1.0">>, package_index, State),
     ?assertEqual(<<"1.1.1">>, Vsn1),
-    {ok, Vsn2} = rebar_packages:find_highest_matching(<<"goodpkg">>, <<"2.0">>, package_index, State),
+    {ok, Vsn2} = rebar_packages:find_highest_matching(
+                   <<"test">>, <<"1.0.0">>, <<"goodpkg">>, <<"2.0">>, package_index, State),
     ?assertEqual(<<"2.0.0">>, Vsn2).