From 384e9e58dba9ff8cf801bfdbe3c2ec406453aa5f Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Tue, 27 Sep 2016 10:23:47 -0400 Subject: Properly support package aliasing and alt names Aliasing only had a bit of ad-hoc support in rebar3, and various issues have encountered problems related to the package names not mapping properly with the application name. One such issue is https://github.com/erlang/rebar3/issues/1290 The problem has been hard to find because it only impacts transitive dependencies (not top-level ones) of other packages. The root cause for this is that the application name was not being tracked by rebar3's internal index, only the package name and its version were. When a given application was a package app, the data for the application name would be reconstructed from the lock file, but only if it were a top-level app or a dependency of a source application where parsing the lock file is necessary to know what comes next. When a transitive dependency of a package dependency was fetched, we instead read its dependencies directly from the in-memory package index within rebar3. This caused us to only read the package name and version, and lost all information regarding application name. This worked fine for most cases since for the vast majority of packages, the package name matches the app name, but failed for all aliases, which would then be moved to directories that wouldn't match the app name. This in turn broke some aspects of code analysis (in Dialyzer), or other functionality relying on static paths, such as including .hrl files from dependencies. This patch reformats the internal storage format of dependencies to align with the internal one used by rebar3, so that the app name can be carried along with the package name and its version. The fix can only work once `rebar3 update` is called so the index is rebuilt internally, and will the file cached on disk will be incompatible with older rebar3 versions. Currently, the following is not covered: - Tests - Including the package hashes of dependencies so they may match what is in a lock file -- they're being `undefined` instead, which may break some lookups. The previous format did not lend itself to hashing in the same way, and it is possible transitive deps were not being tracked properly, or worked by respecting the current package hierarchy. This will require further analysis For now this commit can allow reviewing and discussion. --- test/rebar_deps_SUITE.erl | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'test') diff --git a/test/rebar_deps_SUITE.erl b/test/rebar_deps_SUITE.erl index 24bf2a0..6b2ecea 100644 --- a/test/rebar_deps_SUITE.erl +++ b/test/rebar_deps_SUITE.erl @@ -385,36 +385,36 @@ https_os_proxy_settings(_Config) -> semver_matching_lt(_Config) -> Dep = <<"test">>, - Dep1 = {Dep, <<"1.0.0">>, Dep}, + Dep1 = {Dep, <<"1.0.0">>, Dep, 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">>}], + ?assertEqual([{Dep, {pkg, Dep, <<"0.1.9">>, undefined}}], 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}, + Dep1 = {Dep, <<"1.0.0">>, Dep, 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">>}], + ?assertEqual([{Dep, {pkg, Dep, <<"0.2.0">>, undefined}}], 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}, + Dep1 = {Dep, <<"1.0.0">>, Dep, 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">>}], + ?assertEqual([{Dep, {pkg, Dep, <<"0.2.1">>, undefined}}], 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}, + Dep1 = {Dep, <<"1.0.0">>, Dep, Dep}, MaxVsn = <<"0.2.0">>, Vsns = [<<"0.1.7">>, <<"0.1.9">>, <<"0.1.8">>, <<"0.2.0">>], - ?assertEqual([{Dep, <<"0.2.0">>}], + ?assertEqual([{Dep, {pkg, Dep, <<"0.2.0">>, undefined}}], rebar_prv_update:cmp_(undefined, MaxVsn, Vsns, [], Dep1, fun ec_semver:gt/2)). -- cgit v1.1 From 75920a13a4251336d21bcbdbedb601dcf35de1f7 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Mon, 3 Oct 2016 23:35:58 -0400 Subject: Update existing tests to use new index structure --- test/mock_pkg_resource.erl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'test') diff --git a/test/mock_pkg_resource.erl b/test/mock_pkg_resource.erl index f837713..5769988 100644 --- a/test/mock_pkg_resource.erl +++ b/test/mock_pkg_resource.erl @@ -149,7 +149,10 @@ to_index(AllDeps, Dict) -> ets:new(package_index, [named_table, public]), dict:fold( fun(K, Deps, _) -> - DepsList = [{ec_cnv:to_binary(DK), ec_cnv:to_binary(DV)} || {DK, DV} <- Deps], + DepsList = [{DKB, {pkg, DKB, DVB, undefined}} + || {DK, DV} <- Deps, + DKB <- [ec_cnv:to_binary(DK)], + DVB <- [ec_cnv:to_binary(DV)]], ets:insert(package_index, {K, DepsList, <<"checksum">>}) end, ok, Dict), ets:insert(package_index, {package_index_version, 3}), -- cgit v1.1 From 45d9127dc952430d4b519741cb2303953e302665 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Tue, 4 Oct 2016 10:23:47 -0400 Subject: Add transitive alias tests --- test/rebar_pkg_alias_SUITE.erl | 82 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 75 insertions(+), 7 deletions(-) (limited to 'test') diff --git a/test/rebar_pkg_alias_SUITE.erl b/test/rebar_pkg_alias_SUITE.erl index 8915357..903fdad 100644 --- a/test/rebar_pkg_alias_SUITE.erl +++ b/test/rebar_pkg_alias_SUITE.erl @@ -4,7 +4,7 @@ -include_lib("eunit/include/eunit.hrl"). -include_lib("kernel/include/file.hrl"). -all() -> [same_alias, diff_alias, diff_alias_vsn]. +all() -> [same_alias, diff_alias, diff_alias_vsn, transitive_alias]. %% {uuid, {pkg, uuid}} = uuid %% {uuid, {pkg, alias}} = uuid on disk @@ -32,6 +32,12 @@ init_per_testcase(diff_alias_vsn, Config0) -> AppDir = ?config(apps, Config), rebar_test_utils:create_app(AppDir, "A", "0.0.0", [kernel, stdlib]), RebarConf = rebar_test_utils:create_config(AppDir, [{deps, [{fakelib, "1.0.0", {pkg, goodpkg}}]}]), + [{rebarconfig, RebarConf} | Config]; +init_per_testcase(transitive_alias, Config0) -> + Config = rebar_test_utils:init_rebar_state(Config0,"transitive_alias_vsn_"), + AppDir = ?config(apps, Config), + rebar_test_utils:create_app(AppDir, "A", "0.0.0", [kernel, stdlib]), + RebarConf = rebar_test_utils:create_config(AppDir, [{deps, [{topdep, "1.0.0", {pkg, topdep}}]}]), [{rebarconfig, RebarConf} | Config]. end_per_testcase(_, Config) -> @@ -73,23 +79,73 @@ diff_alias(Config) -> diff_alias_vsn(Config) -> diff_alias(Config). +transitive_alias(Config) -> + %% ensure that the apps fetched under transitive aliases are + %% locked properly, but also that they are stored in the right + %% directory in the build dir to avoid breaking includes and + %% static analysis tools that rely on the location to work + AppDir = ?config(apps, Config), + Lockfile = filename:join([AppDir, "rebar.lock"]), + {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)), + rebar_test_utils:run_and_check( + Config, RebarConfig, ["lock"], + {ok, [{lock, "topdep"},{dep, "topdep"}, + {lock,"transitive_app"},{dep,"transitive_app"}]} + ), + {ok, [{_Vsn, LockData}|_]} = file:consult(Lockfile), + ?assert(lists:any(fun({<<"transitive_app">>,{pkg,<<"transitive">>,_},_}) -> true + ; (_) -> false end, LockData)), + AppDir = ?config(apps, Config), + AliasedName = filename:join([AppDir, "_build", "default", "lib", "transitive_app"]), + PkgName = filename:join([AppDir, "_build", "default", "lib", "transitive"]), + ?assert(filelib:is_dir(AliasedName)), + ?assertNot(filelib:is_dir(PkgName)), + %% An second run yields the same + rebar_test_utils:run_and_check( + Config, RebarConfig, ["lock"], + {ok, [{lock, "topdep"},{dep, "topdep"}, + {lock,"transitive_app"},{dep,"transitive_app"}]} + ), + {ok, [{_Vsn, LockData}|_]} = file:consult(Lockfile), + ?assert(filelib:is_dir(AliasedName)), + ?assertNot(filelib:is_dir(PkgName)), + %% So does an upgrade + rebar_test_utils:run_and_check( + Config, RebarConfig, ["upgrade"], + {ok, [{lock, "topdep"},{dep, "topdep"}, + {lock,"transitive_app"},{dep,"transitive_app"}]} + ), + {ok, [{_Vsn, LockData}|_]} = file:consult(Lockfile), + ?assert(filelib:is_dir(AliasedName)), + ?assertNot(filelib:is_dir(PkgName)), + ok. + mock_config(Name, Config) -> + {ChkFake, Etag} = create_lib(Name, Config, "fakelib"), + {ChkTop, _} = create_lib(Name, Config, "topdep"), + {ChkTrans, _} = create_lib(Name, Config, "transitive_app", "transitive"), Priv = ?config(priv_dir, Config), + TmpDir = filename:join([Priv, "tmp", atom_to_list(Name)]), + %% Add an alias for goodpkg -> fakelib by hand AppDir = filename:join([Priv, "fakelib"]), CacheRoot = filename:join([Priv, "cache", atom_to_list(Name)]), - TmpDir = filename:join([Priv, "tmp", atom_to_list(Name)]), CacheDir = filename:join([CacheRoot, "hex", "com", "test", "packages"]), - filelib:ensure_dir(filename:join([CacheDir, "registry"])), rebar_test_utils:create_app(AppDir, "fakelib", "1.0.0", [kernel, stdlib]), - {Chk,Etag} = rebar_test_utils:package_app(AppDir, CacheDir, "fakelib-1.0.0"), - {Chk,Etag} = rebar_test_utils:package_app(AppDir, CacheDir, "goodpkg-1.0.0"), + {ChkFake, Etag} = rebar_test_utils:package_app(AppDir, CacheDir, "goodpkg-1.0.0"), Tid = ets:new(registry_table, [public]), ets:insert_new(Tid, [ {<<"fakelib">>,[[<<"1.0.0">>]]}, {<<"goodpkg">>,[[<<"1.0.0">>]]}, - {{<<"fakelib">>,<<"1.0.0">>}, [[], Chk, [<<"rebar3">>]]}, - {{<<"goodpkg">>,<<"1.0.0">>}, [[], Chk, [<<"rebar3">>]]} + {<<"topdep">>,[[<<"1.0.0">>]]}, + {<<"transitive">>, [[<<"1.0.0">>]]}, + {{<<"fakelib">>,<<"1.0.0">>}, [[], ChkFake, [<<"rebar3">>]]}, + {{<<"goodpkg">>,<<"1.0.0">>}, [[], ChkFake, [<<"rebar3">>]]}, + {{<<"topdep">>,<<"1.0.0">>}, + [[ + [<<"transitive">>, <<"1.0.0">>, false, <<"transitive_app">>] + ], ChkTop, [<<"rebar3">>]]}, + {{<<"transitive">>,<<"1.0.0">>}, [[], ChkTrans, [<<"rebar3">>]]} ]), ok = ets:tab2file(Tid, filename:join([CacheDir, "registry"])), ets:delete(Tid), @@ -119,3 +175,15 @@ mock_config(Name, Config) -> unmock_config(Config) -> meck:unload(), Config. + +create_lib(Name, Config, PkgName) -> + create_lib(Name, Config, PkgName, PkgName). + +create_lib(Name, Config, AppName, PkgName) -> + Priv = ?config(priv_dir, Config), + AppDir = filename:join([Priv, PkgName]), + CacheRoot = filename:join([Priv, "cache", atom_to_list(Name)]), + CacheDir = filename:join([CacheRoot, "hex", "com", "test", "packages"]), + filelib:ensure_dir(filename:join([CacheDir, "registry"])), + rebar_test_utils:create_app(AppDir, AppName, "1.0.0", [kernel, stdlib]), + rebar_test_utils:package_app(AppDir, CacheDir, PkgName++"-1.0.0"). -- cgit v1.1 From 1f86ed1aed990438103c5f668c4ec930ab637fc9 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Tue, 4 Oct 2016 22:34:22 -0400 Subject: Track package hash in memory index, add hash test This adds tracking of package hash in the in-memory index rather than the current `undefined' values. According to the test added, this is not necessary for transitive package dep hash chcking, but does result in a more complete index search result when doing app lookups, and could yield some optimizations on hash checks by checking from the index structure before fetching a package. --- test/rebar_pkg_alias_SUITE.erl | 44 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) (limited to 'test') diff --git a/test/rebar_pkg_alias_SUITE.erl b/test/rebar_pkg_alias_SUITE.erl index 903fdad..2b8ccd2 100644 --- a/test/rebar_pkg_alias_SUITE.erl +++ b/test/rebar_pkg_alias_SUITE.erl @@ -4,7 +4,8 @@ -include_lib("eunit/include/eunit.hrl"). -include_lib("kernel/include/file.hrl"). -all() -> [same_alias, diff_alias, diff_alias_vsn, transitive_alias]. +all() -> [same_alias, diff_alias, diff_alias_vsn, transitive_alias, + transitive_hash_mismatch]. %% {uuid, {pkg, uuid}} = uuid %% {uuid, {pkg, alias}} = uuid on disk @@ -38,6 +39,12 @@ init_per_testcase(transitive_alias, Config0) -> AppDir = ?config(apps, Config), rebar_test_utils:create_app(AppDir, "A", "0.0.0", [kernel, stdlib]), RebarConf = rebar_test_utils:create_config(AppDir, [{deps, [{topdep, "1.0.0", {pkg, topdep}}]}]), + [{rebarconfig, RebarConf} | Config]; +init_per_testcase(transitive_hash_mismatch, Config0) -> + Config = rebar_test_utils:init_rebar_state(Config0,"transitive_alias_vsn_"), + AppDir = ?config(apps, Config), + rebar_test_utils:create_app(AppDir, "A", "0.0.0", [kernel, stdlib]), + RebarConf = rebar_test_utils:create_config(AppDir, [{deps, [{topdep, "1.0.0", {pkg, topdep}}]}]), [{rebarconfig, RebarConf} | Config]. end_per_testcase(_, Config) -> @@ -120,6 +127,41 @@ transitive_alias(Config) -> ?assertNot(filelib:is_dir(PkgName)), ok. +transitive_hash_mismatch(Config) -> + %% ensure that the apps fetched under transitive aliases are + %% locked properly, but also that they are stored in the right + %% directory in the build dir to avoid breaking includes and + %% static analysis tools that rely on the location to work + AppDir = ?config(apps, Config), + Lockfile = filename:join([AppDir, "rebar.lock"]), + {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)), + rebar_test_utils:run_and_check( + Config, RebarConfig, ["lock"], + {ok, [{lock, "topdep"},{dep, "topdep"}, + {lock,"transitive_app"},{dep,"transitive_app"}]} + ), + {ok, [LockData|Attrs]} = file:consult(Lockfile), + %% Change Lock hash data to cause a failure next time, but on transitive + %% deps only + NewLock = [LockData|lists:map( + fun([{pkg_hash, Hashes}|Rest]) -> + [{pkg_hash, [{<<"transitive_app">>, <<"fakehash">>} + | lists:keydelete(<<"transitive_app">>, 1, Hashes)]} + | Rest] + ; (Attr) -> + Attr + end, Attrs)], + {ok, Io} = file:open(Lockfile, [write]), + [io:format(Io, "~p.~n", [Attr]) || Attr <- NewLock], + file:close(Io), + ct:pal("lock: ~p", [file:consult(Lockfile)]), + ec_file:remove(filename:join([AppDir, "_build"]), [recursive]), + ?assertMatch( + {error, {rebar_fetch, {unexpected_hash, _, _, _}}}, + rebar_test_utils:run_and_check(Config, RebarConfig, ["lock"], return) + ), + ok. + mock_config(Name, Config) -> {ChkFake, Etag} = create_lib(Name, Config, "fakelib"), {ChkTop, _} = create_lib(Name, Config, "topdep"), -- cgit v1.1