jelly-beam / rebar3_ex_doc

rebar3 plugin for generating docs with ex_doc
Apache License 2.0
43 stars 13 forks source link

Can't find include file in an umbrella project #3

Closed altaica closed 2 years ago

altaica commented 2 years ago

I tried this on an umbrella codebase and it struggled due to modules with an explicit include path to another application. For example:

Project
+--apps
   +--app1
   |  +--include
   |  |  +--app1.hrl
   |  +--src
   |     +--app1.erl
   +--app2
      +--src
         +--app2.erl
-module(app2).
-include("apps/app1/include/app1.hrl").

This is fine for compile, ct, xref, dialyzer, elvis, hank - the usual suite. Also fine for edoc. However, with rebar3 ex_doc I get:

can't find include file "apps/app1/include/app1.hrl"
edoc: error in doclet 'edoc_doclet_chunks': {'EXIT',error}.
starbelly commented 2 years ago

Looks like this might be a problem with edoc when generating chunks. I get the same error with edoc using the following config :

{edoc_opts, [
    {preprocess, true},
    {doclet, edoc_doclet_chunks},
    {layout, edoc_layout_chunks},
    {dir, "_build/docs"},
    private,
    hidden
]}.

It could also be there's some extra opts we need to set when calling edoc , as another possibility.

Edit:

Indeed, if I remove all options sans preprocess same failure, it can't find the include file. Really it's erlc I believe that can't find the include file.

starbelly commented 2 years ago

A few thoughts :

  1. As a quick fix, you could use relative paths (i.e., "../../app1/include/app1.hrl"). I believe I've seen that in OTP, regardless I can say that works.

  2. It's not clear who's responsibility this is. I can think a patch that could go up for epp, if OTP would accept it. Basically, for any include file you get the abs path for it, likewise you transform include paths provided via includes option to edoc into abs paths. I think that would work, but wouldn't know unless it was tried. I've never poked around in edoc or epp in anger.

I could also see where possibly the rebar3 edoc provider could handle this.

Regardless of the outcome of 2, we will have to support merging edoc opts I believe, or allow specifying them within ex_doc opts, the latter might favorable if there's a case where you want ex_doc but you maybe also need edoc and need different sets of options. You could probably use profiles to solve that problem too though.

In the end, in regards to 2. . we shall have to reach out to a few folks and figure out where the responsibility lies.

paulo-ferraz-oliveira commented 2 years ago

@starbelly, I'm experiencing the same issue. Using your 1. workaround (../stuff.hrl) I was able to bypass the issue while maintaining the lib. compilable.

erszcz commented 2 years ago

Hi, @altaica!

It works fine if you use -include_lib to include the header files across apps. Please see https://github.com/erszcz/learning/blob/482c46d20a907cc541c6cd4a7648721d6d5fdbf1/rebar3-umbrella-test/apps/test2/src/test2_app.erl#L13 for an example umbrella repo.

The problem with -include and include path handling is that each app parsing source code (so Rebar3, edoc, compiler, Dialyzer, ...) have to deal with -I flags and include paths separately, which is a lot of redundant work and lot of space for inconsistencies to sneak in. With -include_lib we reuse the code path handling infrastructure from stdlib, so do away with all this redundancy. It's better to stick with -include_lib if only possible.

I hope this helps!

paulo-ferraz-oliveira commented 2 years ago

@erszcz, my issue is a bit different.

My app is like

+--my_app
   +--src
   |  +--drivers
   |     +--my_app_driver.erl
   +--my_app.hrl

Compilation is fine with just -include("my_app.erl"). (from my_app_driver.erl), but rebar3_ex_doc doesn't like it.

Using -include(".../my_app.hrl") the problem is somewhat solved, though I'm sure this'll break if I move my_app_driver.erl to a different location.

Thoughts?

Note: I'm using erl_opts with {i, "src"}, in rebar.config.

erszcz commented 2 years ago

@paulo-ferraz-oliveira Got it, include_lib doesn't work with that. Why put the header file in the root dir of the app instead of include, though?

I'm not sure about rebar3_ex_doc, but with regard to EDoc itself it's possible to pass {includes, [Dir1, Dir1, ...]} as documented at https://www.erlang.org/doc/man/edoc.html#read_source-2. It could be added to edoc_opts in rebar.config:

{edoc_opts, [
    {preprocess, true},
    {includes, Dirs},
    ...
]}.

I think it's sensible to leave setting includes properly to the user as it's not possible to foresee every header file placement for every project out there. The standard and advised way, though, is to use include_lib.

paulo-ferraz-oliveira commented 2 years ago

Why put the header file in the root dir of the app instead of include, though?

I don't want the file to be importable by other lib.s.

Generating the doc. with edoc I didn't have that issue. And I did use includes.

I see, though, it's possible (https://github.com/miniclip/gen_rpc/pull/7/files#diff-427d8ddce21ccc4fae24fb547c7a82c2a793ed57208d8343e519e0aec8119ff7L122) the removal of edoc's options caused the issue, but I'll have to check that again.

paulo-ferraz-oliveira commented 2 years ago
➜  gen_rpc git:(feature/rebar3_ex_doc) ✗ rebar3 compile
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling gen_rpc
➜  gen_rpc git:(feature/rebar3_ex_doc) ✗ rebar3 edoc
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling gen_rpc
===> Running edoc for gen_rpc
➜  gen_rpc git:(feature/rebar3_ex_doc) ✗ rebar3 hex publish docs --dry-run
===> Verifying dependencies...
===> docs argument given, will not publish package
===> Analyzing applications...
===> Compiling gen_rpc
===> Running edoc for gen_rpc
.../gen_rpc/src/driver/gen_rpc_driver_tcp.erl: at line 19: can't find include file "app.hrl"
edoc: error in doclet 'edoc_doclet_chunks': {'EXIT',error}.
===> An unknown error occurred generating doc chunks with edoc. Run with DIAGNOSTICS=1 for more details.

This is with the includes option set in place for edoc.

paulo-ferraz-oliveira commented 2 years ago

@starbelly, based on what I believe is @erszcz's suggestion (thanks for that), I hacked a includes option for ex_doc (hard-coded for testing) and it worked. Would it be possible to read it from the config. and use it in

    EdocOpts = [
        {preprocess, true},
        {doclet, edoc_doclet_chunks},
        {layout, edoc_layout_chunks},
        {includes, Includes},
        %          ^here
        {dir, OutDir},
        private,
        hidden
    ],

I think I read, somewhere else, that there's a goal of merging {edoc_opts, _} (I'm not sure if it's for a transitional period or permanent) into this, but I'm not sure, so am leaving the suggestion.

erszcz commented 2 years ago

Exactly, the same way you provide it to the compiler:

I'm using erl_opts with {i, "src"}, in rebar.config.

We have to provide it to EDoc, as it's a separate app:

{edoc_opts, [
    {preprocess, true},
    {includes, ["src"]},
    ...
]}.

BTW, this is how the option is populated in edoc_cli.erl: https://github.com/erlang/otp/blob/56240f084d80de80d5f1c7fef94db68a6f1b81cc/lib/edoc/src/edoc_cli.erl#L105

I don't want the file to be importable by other libs.

Ahh, ok, all clear. Sorry for the confusion, then. From this tree:

+--my_app
   +--src
   |  +--drivers
   |     +--my_app_driver.erl
   +--my_app.hrl

I understood that it's my_app/my_app.hrl, whereas in fact it seems to be my_app/src/my_app.hrl.

Would it be possible to read it from the config and use it in [EdocOpts...]

I think we could even default to adding the $APP/include and $APP/src to EDoc's {include, Dirs}. It shouldn't hurt and would likely solve 90% of header placement issues.

I hope it helps!

starbelly commented 2 years ago

I think I read, somewhere else, that there's a goal of merging {edoc_opts, _} (I'm not sure if it's for a transitional period or permanent) into this, but I'm not sure, so am leaving the suggestion.

Do you mean in rebar3 edoc provider? That could possibly work. It's been a minute since I've tried, but I seem to remember having trouble with edoc devoid of this plugin anyway and it was only when preprocess was set to true. That said, we should be merging the opts anyway.

I think we could even default to adding the $APP/include and $APP/src to EDoc's {include, Dirs}. It shouldn't hurt and would likely solve 90% of header placement issues.

Worth trying.

paulo-ferraz-oliveira commented 2 years ago

I mean: edoc_opts already supports it and it works fine. If we somehow can import that (or create a new place for the option) in ex_doc that'd be nice, since we fix the bug I mention.

That said, we should be merging the opts anyway.

If this is the case, it'll even be simpler, not only for a migration period (since consumers don't have to move stuff away from ex_doc). In any case, I'm wondering if allowing the includes option directly inside ex_doc isn't an option, for you (?)

Also, I like @erszcz's suggestion of having the default includes include ["include", "src"] already (I tend to repeat this in all my lib.s, but I'm not sure it's a generic use case).

I'll probably get to pull request and we can go from there 😄