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.

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

[#762] add include dir option of deps for compiler diagnostics #764

Closed
wants to merge 2 commits into from

Conversation

luos
Copy link
Contributor

@luos luos commented Jun 10, 2020

If an erlang file has a hrl included the compiler diagnostics raises an
error because it is not included in the arguments for the compile call.

Fix needs that the dependency directory is configured correctly in the erlang_ls.config file.

Related #762

If an erlang file has a hrl included the compiler diagnostics raises an
error because it is not included in the operation.

Fix needs that the dependency directory is configured correctly.

Related erlang-ls#762
@luos luos changed the title add include dir option of deps for compiler diagnostics [#762] add include dir option of deps for compiler diagnostics Jun 10, 2020
Copy link
Contributor

@alanz alanz left a comment

Choose a reason for hiding this comment

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

Can you add a test to show this usage?

@luos
Copy link
Contributor Author

luos commented Jun 11, 2020

Hi,

Should I add an example project under priv?

@alanz
Copy link
Contributor

alanz commented Jun 11, 2020

Should I add an example project under priv?

If possible do it under tests/els_diagnostics_SUITE_data/. We only use priv for stuff that is so broken that rebar cant use it.

Or, modify one of the existing projects under priv. But probably better to add a fresh one, for the specific problem.

@luos luos force-pushed the add_include_dirs_to_compile branch from 10912a2 to fe80b56 Compare June 11, 2020 16:10
@luos
Copy link
Contributor Author

luos commented Jun 11, 2020

Okay, thanks for the answer, added a test.

I see there is an other option for include_dirs, but I think deps_dirs fits better because, for example Erlang.mk uses the deps folder, and the header usually (in my case) comes from a dependency.

Copy link
Contributor

@alanz alanz left a comment

Choose a reason for hiding this comment

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

LGTM

@alanz alanz requested a review from robertoaloi June 11, 2020 16:27
@robertoaloi
Copy link
Member

Hi @luos This feature should already be available, but is configuration is probably a bit tricky. Something like this should work for your case:

include_dirs:
  - "deps/*/include"
  - "deps"

But we could consider this to avoid the manual step. Any thought @jfacorro ?

Also, this should all go away once we integrate with the BSP part.

@luos
Copy link
Contributor Author

luos commented Jun 15, 2020

Okay, thanks, I'll check it out and report back if it works with that settings.

Yes, it would be better to automagically pick up Erlang MK / Rebar style structure, but if it needs to be configured that is fine as well. :-)

Thanks

@robertoaloi
Copy link
Member

@luos This PR has been sitting here for a huge amount of time, so sorry about that! Is this change something you still need? Should we solve conflicts?

@luos
Copy link
Contributor Author

luos commented Jan 11, 2021

Hi,

Let me check it again, I believe maybe it was solved in one of the newer versions as I have not run into this since.

Thanks,

@luos
Copy link
Contributor Author

luos commented Jan 19, 2021

Hi,

Sorry for the late reply. It still does not work for me.

Pull this repo:
https://github.com/rabbitmq/rabbitmq-metronome/

Pull the deps by running make or gmake, probably it will get stuck but the relevant dep is downloaded fine.

My erlang_ls.config file is the following:

include_dirs:
  - "deps/*/include"
  - "deps"
  - "include"

The error I get is this:

kép

kép

Probably as a fix isntead of introducing dep_dirs the include_dirs option should be passed in to the compiler because the problem here is that the code loads the modules but that only works for modules, and a hrl is not a module.

As I see currently include_dirs is only passed in to dialyzer.

Thanks

Base automatically changed from master to main January 24, 2021 08:26
@robertoaloi
Copy link
Member

Hi! The following should work:

deps_dirs:
  - "deps/*"
include_dirs:
  - "deps"
  - "deps/*/include"
  - "include"

I know, it's awkward right now. One entry is for the include, the other one for include_lib

@robertoaloi
Copy link
Member

Closing for lack of activity. The proposed solution should work without requiring changes. If that's not the case, feel free to reopen.

@luos luos deleted the add_include_dirs_to_compile branch February 23, 2021 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants