-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Always adding :debug_info on erlc_options #9348
Conversation
That way, if someone sets erlc_options to `[warnings_as_errors: true]`, `:debug_info` would still be on the options. To configure it to not save debug info, one should only add `debug_info: false` to erlc_options
I also fixed a doc that was pointing to the wrong default value for |
I'm checking how to add tests for this now. Also, there should be a changelog notice or something like that, since the change breaks compatibility. e.g. if anyone was just dismissing |
So as it is done now, I'm not sure there is any good way of testing it, since it is an inner logic inside |
You should be able to test it like this: https://github.com/elixir-lang/elixir/blob/master/lib/mix/test/mix/tasks/compile.erlang_test.exs#L14 |
Yeah, I saw that, but what I couldn't figure out yet is how to check if the |
@kelvinst it should be possible to check the generated files with |
Cool! Thanks @michalmuskala! Really helpful tip (I suppose you mean |
PS.: I just realized that I added the default options to the wrong place I guess, that way it will apply it only for the |
Actually, nevermind, just realized |
Right, so the problem was that I was just running |
{:ok, {module, [debug_info: {:debug_info_v1, backend, data}]}} = | ||
:beam_lib.chunks(binary, [:debug_info]) | ||
|
||
assert backend.debug_info(:elixir_v1, module, data, []) != {:error, :missing} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should assert with :erlang_v1
- after all it's compiling erlang modules, so it's kind of expected it won't be able to return elixir debug info.
assert backend.debug_info(:elixir_v1, module, data, []) != {:error, :missing} | |
assert backend.debug_info(:erlang_v1, module, data, []) != {:error, :missing} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't even assert this. Asserting it returns a debug info chunk is probably good enough!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean something like this?
{:ok, {_, [debug_info: {:debug_info_v1, _, {info, _}}]}} = :beam_lib.chunks(binary, [:debug_info])
assert info != :none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked and the only difference is that it returns :none
for that part when debug info is disabled, instead of the list of attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have started with that actually, just changed to use backend.debug_info
because I was afraid that maybe the format could change in the future and then the test could fail. Very unlikely, but who knows 🤷♂
+ better docs for `erlc_options`
The point is that if they change the format, this test should not fail. It
is ok for them to do that. As long as there is something, we are good.
--
*José Valimwww.plataformatec.com.br
<http://www.plataformatec.com.br/>Founder and Director of R&D*
|
erlc_options: [:debug_info, {:i, 'path/to/include'}] | ||
The following options are always added to `:erlc_options` when running the compiler: | ||
|
||
[:debug_info, :return, :report, outdir: compile_path, i: include_path] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return and report are implementation details. If the user changes them, then the compiler will fail. I would remove this section (or talk about only debug_info).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks!! I will comment only about debug_info.
@@ -729,7 +729,7 @@ defmodule Mix.Project do | |||
elixirc_paths: ["lib"], | |||
erlc_paths: ["src"], | |||
erlc_include_path: "include", | |||
erlc_options: [:debug_info], | |||
erlc_options: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be empty since we already add it everytime when compiling erlang.
erlc_options: [:debug_info, {:i, 'path/to/include'}] | ||
The following options are always added to `:erlc_options` when running the compiler: | ||
|
||
[:debug_info, :return, :report, outdir: compile_path, i: include_path] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks!! I will comment only about debug_info.
Thanks @kelvinst! 💜 |
That way, if someone sets erlc_options to
[warnings_as_errors: true]
,:debug_info
would still be on the options. To configure it to not save debug info, one should only adddebug_info: false
to erlc_optionsCloses #9184