Dialyzer could not find git_subdir application

Kuroneer commented 4 years ago

Environment

Report:

$ rebar3 report dialyzer
Rebar3 report
 version 0.0.0
 generated at 2020-09-01T22:12:06+00:00
=================
Please submit this along with your issue at https://github.com/erlang/rebar3/issues (and feel free to edit out private information, if any)
-----------------
Task: dialyzer
Entered as:
  dialyzer
-----------------
Operating System: x86_64-pc-linux-gnu
ERTS: Erlang/OTP 22 [erts-10.7.2.3] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:1] [hipe]
Root Directory: /usr/local/lib/erlang
Library directory: /usr/local/lib/erlang/lib
-----------------
Loaded Applications:
bbmustache: 1.10.0
certifi: 2.5.2
cf: 0.3.1
common_test: 1.18.2
compiler: 7.5.4.1
crypto: 4.6.5.1
cth_readable: 1.4.8
dialyzer: 4.1.1
edoc: 0.11
erlware_commons: 1.3.1
eunit: 2.4.1
eunit_formatters: 0.5.0
getopt: 1.0.1
hipe: 3.19.3
inets: 7.1.3.3
kernel: 6.5.2.1
providers: 1.8.1
public_key: 1.7.2
relx: 4.0.0
sasl: 3.4.2
snmp: 5.5.0.2
ssl_verify_fun: 1.1.6
stdlib: 3.12.1
syntax_tools: 2.2.1
tools: 3.3.1

-----------------
Escript path: /usr/local/bin/rebar3
Providers:
  app_discovery as clean compile compile cover ct deps dialyzer do edoc escriptize eunit get-deps help install install_deps list lock new path pkgs release relup report repos shell state tar tree unlock unlock_platform update upgrade upgrade upgrade upgrade_platform version warn_outdated_deps warn_outdated_deps_abort xref

Rebar3 version: 3.14.0

Relevant dependency: tuenti_config application, which is defined as a git_subdir:
```
 [{tuenti_config,
      {git_subdir,"git-repo-url",
          {tag,"erlang-xconfig-0.3.3"},
          "bindings/erlang"}},
```
Unfortunately, I cannot provide the actual code (neither library nor project), but tomorrow I'll set up a dummy OS project where this is shown.

Current behaviour

$DEBUG=1 rebar3 dialyzer
... (omitted)
===> Resolving files...
... (omitted)
===> rivets modules: [rivets_worker,rivets,rivets_relay,rivets_hook]
===> statsd_config modules: [statsd,statsd_geigerl_config]
===> stdlib modules: [zip,dets_sup,timer,supervisor,proc_lib,io_lib,
                             gen_server,filelib,file_sorter,erl_tar,erl_error,
                             dets_server,calendar,beam_lib,sys,sofs,sets,qlc,
                             proplists,ordsets,maps,lists,gb_trees,gb_sets,
                             eval_bits,ets,erl_expand_records,erl_bits,
                             digraph_utils,dets_utils,dets,c,shell_default,
                             rand,qlc_pt,orddict,io_lib_fread,io_lib_format,
                             gen_event,escript,erl_lint,edlin_expand,dict,
                             base64,unicode,supervisor_bridge,slave,re,queue,
                             filename,erl_parse,erl_compile,erl_abstract_code,
                             win32reg,uri_string,string,erl_posix_msg,math,
                             log_mf_h,error_logger_tty_h,error_logger_file_h,
                             digraph,io_lib_pretty,dets_v9,binary,array,
                             unicode_util,shell,pool,otp_internal,
                             erl_internal,erl_eval,random,ms_transform,
                             gen_statem,gen_fsm,gen,erl_scan,edlin,io,
                             erl_anno,erl_pp,epp]
===> taskerl modules: [taskerl_gen_server_serializer,taskerl]
===> Could not find application: tuenti_config

Expected behaviour

I'd expect to find the application and continue with the dialyzer task

Triage

It seems that https://github.com/erlang/rebar3/tree/master/src/rebar_prv_dialyzer.erl#L267-L268 returns {error, bad_name}, I guess that it's because the .app is in _build/default/lib/tuenti_config/bindings/erlang/ebin. I think that it'd be possible to obtain the directory from the app_info somehow, but the state at that point does not have the app_info for the dependencies (at least that I found)

Kuroneer commented 4 years ago

As promised, using the minimal repository found in Kuroneer/rebar314_dialyzer_test will show this problem when running rebar3 dialyzer

tsloughter commented 4 years ago

Perfect, thanks! I can reproduce and am digging into it shortly. I bet its simply how we reference app directories in dialyzer not using the right attribute on an app_info.

tsloughter commented 4 years ago

Hm, yea, it does code:lib_dir(AppName, ebin), not sure if the fix is to use app_info functions for apps or to figure out why the it appears git_subdir deps aren't in the code path, which they should be.

tsloughter commented 4 years ago

@ferd you know the rebar_paths code best, any quick idea why this would be happening? Does it mean the app_info ebin_dir is actually wrong since that is what rebar_paths uses to setup the code path?

pablocostass commented 4 years ago

@tsloughter I was checking out this issue too and went with the route of using app_info functions. Let's wait to see what Fred has to say, but rebar_app_info:ebin_dir/1 should not be wrong as I'm getting the correct ebin path for the git_subdir dep both by getting it myself and by printing the ebin paths obtained at rebar_paths :/

tsloughter commented 4 years ago

@pablocostass interesting. I just did a dump of AppGroups in rebar_paths:set_paths and noticed the ebin_dir is wrong:

"/home/tristan/Devel/rebar314_dialyzer_test/_build/default/lib/my_awesome_dep",
"/home/tristan/Devel/rebar314_dialyzer_test/_build/default/lib/my_awesome_dep/bindings/erlang",
"/home/tristan/Devel/rebar314_dialyzer_test/_build/default/lib/my_awesome_dep/bindings/erlang",
undefined,

That is the fetch_dir, dir, out_dir and ebin_dir fields of the app info record.

tsloughter commented 4 years ago

Ah, I guess rebar_app_info:ebin_dir/1 should still return the correct value despite being undefined because the code is:

-spec ebin_dir(t()) -> file:name().
ebin_dir(#app_info_t{ebin_dir=undefined,
                     out_dir=OutDir}) ->
    filename:join(rebar_utils:to_list(OutDir), "ebin");
ebin_dir(#app_info_t{ebin_dir=EbinDir}) ->
    EbinDir.

ferd commented 4 years ago

The rebar_app_info:ebin_dir is correct if I instrument rebar_paths, and also what I get from calling code:get_path() from the shell. But looking up the path in the code server does not work.

Looking at the code server's ETS table (code_names), nothing is in there for my_awesome_dep even if service_discovery is there when nothing is started.

I looked at the code server and the thing is that it expects the name above ebin in the directory hierarchy to be the app name... in this case it's erlang. See the [elided] trace:

14:3:45.359017 <0.50.0> code_server:insert_name("erlang", "/tmp/rebar314_dialyzer_test/_build/default/lib/my_awesome_dep/bindings/erlang/ebin", code_names)   
14:3:45.359447 <0.50.0> code_server:del_ebin("/tmp/rebar314_dialyzer_test/_build/default/lib/my_awesome_dep/bindings/erlang/ebin")
14:3:45.365748 <0.50.0> code_server:del_ebin/1 --> "/tmp/rebar314_dialyzer_test/_build/default/lib/my_awesome_dep/bindings/erlang"    
14:3:45.365888 <0.50.0> code_server:do_insert_name("erlang", "/tmp/rebar314_dialyzer_test/_build/default/lib/my_awesome_dep/bindings/erlang", code_names)

and the confirmation:

12> ets:lookup(code_names, "erlang").
[{"erlang",
  "/tmp/rebar314_dialyzer_test/_build/default/lib/my_awesome_dep/bindings/erlang",
  "erlang",[]}]

To work well, the subdir stuff appears to require eliding paths so that the ebin/ directory will be under a directory name that is exactly the application name.

tsloughter commented 4 years ago

Ahh. well crap.

ferd commented 4 years ago

Yeah. I guess rebar3 took care of it for so long we forgot these small things are hard implicit requirements

tsloughter commented 4 years ago

Yup :(

pablocostass commented 4 years ago

A dirty workaround as in checking for any git_subdir app and getting its ebin dir with rebar_app_info:ebin_dir/1 would be okay?

tsloughter commented 4 years ago

So couple decisions to be made:

Can we fix dialyzer by working off of AppInfos and using app_info:ebin instead of code:lib_dir? It only cares about modules, right? So it won't be confused by the erlang app name in the code path?
Should we create a different directory for the subdir dep to be compiled to so that it matches? I think this may be hard because there are other app dirs like priv.
Document a requirement that the subdir be the name of the application and error out if it isn't.

ferd commented 4 years ago

I think we should work things in a way that the VM expects them because there's probably a lot of other stuff using the same assumptions elsewhere. I'd say 2 and 3 are the only way to go

Specifically, git_subdir needs to take the subdirectory specified, just take whatever's inside there, and shove it in a _build/<profile>/lib/<appname>/ directory. This removes the ability to go look within parents of the subdir for data and whatnot, but I can't think of another way that would respect OTP requirements through and through.

Kuroneer commented 4 years ago

Specifically, git_subdir needs to take the subdirectory specified, just take whatever's inside there, and shove it in a _build/<profile>/lib/<appname>/ directory. This removes the ability to go look within parents of the subdir for data and whatnot, but I can't think of another way that would respect OTP requirements through and through.

The problem with that approach is that the root of the dependency (with the .git directory) is itself the one that's in _build/<profile>/lib/<appname>/, so maybe the dependency itself should be cloned elsewhere.

ferd commented 4 years ago

ah yeah, that prevents the {vsn, git} stuff from working if we drop it :/

I guess that points to needing to move the priv/ and ebin/ directories (and their content) to the root of the project then, and keeping src/ where it is, which will fake everything into being into the right spot

tsloughter commented 4 years ago

Right. That is why adding subdirs wasn't done earlier :(.

ferd commented 4 years ago

Yeah so I'm thinking:

move $dep/$subdir/ebin to $dep/ebin
move $dep/$subdir/priv to $dep/priv
symlink $dep/priv and $dep/ebin to their proper root

The latter is likely risky, but better than doing nothing if there are hooks and stuff on which the app relies. Also may break things on windows when they don't have symlink permissions, but a bunch of stuff already breaks there in that case?

This also possibly does not fit well at all into our separation of source provider and app handling though :(

Kuroneer commented 4 years ago

Moving (or linking) dirs within the dependency is bound to cause problems with git. What happens if by any chance the git project has a ebin dir in the root?. Also, with git pull and family, if the dependency is upgraded, it may refuse to pull.

In my opinion the cleanest solution would be to clone the dependency somewhere else and then link/copy the subdir in _build/<profile>/lib/<appname>/, but that complicates a lot the tracking of deps

tsloughter commented 4 years ago

I think the least likely to cause more problems and shouldn't be an issue since this is a new feature is to require the subdir be the name of the app.

ferd commented 4 years ago

We're probably stuck documenting the limitation and moving on at this point yeah. Require the last subdir to be the name of the app and that's it. We can possibly do a check and warn to make it friendly rather than just docs too.

tsloughter commented 4 years ago

Right, I'm going to add a check to verify it when downloading the dep so it can error out and tell you this won't work.

tsloughter commented 4 years ago

Should we append the app name to the path?

Meaning {appname, {git_subdir, "...", {branch, "master"}, "bindings/erlang/"}} would use directory bindings/erlang/appname.

Kuroneer commented 4 years ago

I've verified that with the subdir name changed to the app name everything works fine (if anyone is interested, the test repository is the same, branch rename_dep_subdir)

Should we append the app name to the path?

Meaning {appname, {git_subdir, "...", {branch, "master"}, "bindings/erlang/"}} would use directory bindings/erlang/appname.

I think that's a good idea because it fixes the problem by design. The only problem being that in a few month's time someone will wonder why is there such a limitation. I suppose that a comment somewhere is enough.

ferd commented 4 years ago

I'm not sure that adding a directory is any better than removing them from that point of view. It'll still mess with some of the stuff (i.e. parts of paths somewhat hardcoded in hooks), risk clashing, and so on.

Kuroneer commented 4 years ago

I'm not sure that adding a directory is any better than removing them from that point of view. It'll still mess with some of the stuff (i.e. parts of paths somewhat hardcoded in hooks), risk clashing, and so on.

I think that @tsloughter means only in the config, since you already have appname, and you restrict the subdir name, you don't need the subdir path to be foo/bar/appname, just foo/bar

ferd commented 4 years ago

Oh yeah, makes sense. That went over my head. We should look for it and that helps warn easily while obvious cutting down on errors for typoes.

tsloughter commented 4 years ago

Right, removes duplication. I'm working on it now.

tsloughter commented 4 years ago

Alright, basically have it done, but have to work. I'll get it cleaned up and test added at some point today.

Kuroneer commented 4 years ago

Thanks a lot

erlang / rebar3