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

Remove debug_info option when no_debug_info option set #1653

Closed

Conversation

bitnitdit
Copy link

No description provided.

All of the following options will be removed when no_debug_info option set

	encrypt_debug_info
	{debug_info, {Backend, Data}}
	{debug_info_key, KeyString}
	{debug_info_key, {Mode, KeyString}}
@ferd
Copy link
Collaborator

ferd commented Oct 31, 2017

I'm not quite sure why you would add no_debug_info rather than removing the bad options. Is there a use case you have in mind specifically here? Overrides or profiles-related in any way? I'm also a bit confused about why this apparently killed all existing test runs.

@bitnitdit
Copy link
Author

bitnitdit commented Oct 31, 2017

Using rebar3 to create a foo release with the default setting, run $ rebar3 as prod release, the .beam files generated are all compiled with debug_info.

I want to remove the debug_info compile option. So in rebar.config file, adding the no_debug_info option in the prod profile as the following:

{profiles, [{prod, [{erl_opts, [no_debug_info]},
                    {relx, [{dev_mode, false},{include_erts, true}]}]
            }]
}.

then run $ rebar3 as prod release again, but this time the .beam files generated are all compiled with debug_info as well. That is to say, no_debug_info option in prod profile does not work.

The only way to remove the debug_info is to delete the first line, {erl_opts, [debug_info]}., in rebar.config file. But IMHO the profile settings should have higher priority.

@bitnitdit
Copy link
Author

bitnitdit commented Oct 31, 2017

Just walk through the following lines,

erl_opts(Opts) ->
    RawErlOpts = filter_defines(?MODULE:get(Opts, erl_opts, []), []),
    Defines = [{d, list_to_atom(D)} ||
                  D <- ?MODULE:get(Opts, defines, [])],
    AllOpts = Defines ++ RawErlOpts,
    case proplists:is_defined(no_debug_info, AllOpts) of
        true ->
            [O || O <- AllOpts, O =/= no_debug_info];
        false ->
            [debug_info|AllOpts]
    end.

we can see that no_debug_info option does not do anything to any other options, its usage is only remove itself. So no_debug_info option is useless, I think it's not the designed semantic for it.

@ferd
Copy link
Collaborator

ferd commented Oct 31, 2017

Since debug_info is the default behaviour, you could just remove debug_info from the default profile and it would all work.

@bitnitdit
Copy link
Author

bitnitdit commented Nov 1, 2017

Yes, just remove debug_info from the default profile is working.

But what's the point of the no_debug_info option? Why we have a useless option no_debug_info there?

@ferd
Copy link
Collaborator

ferd commented Nov 1, 2017

The no_debug_info turns off the rebar behaviour of inserting debug_info by default in compile options, basically.

@ferd
Copy link
Collaborator

ferd commented Nov 1, 2017

To be clear, I'm not against the patch, but it would at least need to keep tests working.

@bitnitdit
Copy link
Author

bitnitdit commented Nov 1, 2017

From the link https://travis-ci.org/erlang/rebar3/jobs/295167011:

... ...
%%% rebar_as_SUITE ==> as_with_task_args: OK

=ERROR REPORT==== 31-Oct-2017::05:08:23 ===
** Generic server rebar_log_meck terminating 
** Last message in was {'EXIT',<0.2858.0>,
                        {#Ref<0.688754590.3314286593.37531>,27911,
                         {'EXIT',
                          {undef,
                           [{rebar_log_meck_original,default_level,[],[]},
                            {rebar_log,default_level,[],[]},
                            {rebar3,log_level,0,
                             [{file,
                               "/home/travis/build/erlang/rebar3/_build/test/lib/rebar/src/rebar3.erl"},
                              {line,258}]},    
... ...

It seems like the failed test is caused by the function rebar_log_meck_original:default_level was undefined; not caused by this PR's code change.

In my local env, the $ ./bootstrap && ./rebar3 ct test can pass smoothly with the same code change.

I can not have any idea why the test failed on travis-ci. So sorry.

@ferd
Copy link
Collaborator

ferd commented Nov 1, 2017

I'll try to look into it today then. That's odd.

@ferd
Copy link
Collaborator

ferd commented Nov 5, 2017

So I have figured why tests explode...

Currently the behaviour is that:

  • you have debug_info on (by default) and then turn it off with no_debug_info, you get no debug info
  • you have debug_info on explicitly and then turn it off with no_debug_info, you keep getting debug info

The current rebar3's test suites depend on the later: we publish in the default profile without debug_info to keep the size of the executables small. We then use profile options to turn debug_info on for dialyzer or Common Test (and meck uses that debug info to work).

By modifying the feature the way this PR does it, when no_debug_info is used in the default profile, then follow-up profiles cannot ever enable it either.

This PR just happens to reverse the problem you experienced, and in doing so, breaks rebar3's test suite itself.

I need to think of what could be a good way to fix this to work well with profiles. So far I think the best would work by honoring whichever is the first found between no_debug_info or any of the debug_info-related entries in the list and removing the related terms in the trailing end of the erl_opts list to ensure nice playing with profiles. I'm not sure what corner-cases this may hide.

@bitnitdit
Copy link
Author

@ferd , thanks!

Sorry for this PR is not good enough to cover the case you figured out. :-(

@ferd
Copy link
Collaborator

ferd commented Nov 6, 2017

It's fine anyway. I think there's a good way forward from here anyway. I'll try to open up an alternative PR that you can try to see if it works well for you.

@ferd ferd mentioned this pull request Nov 6, 2017
@ferd
Copy link
Collaborator

ferd commented Nov 6, 2017

Attempt at #1656

@bitnitdit
Copy link
Author

bitnitdit commented Nov 15, 2017

@ferd , the change in #1656 looks good to me. Thanks!

@bitnitdit
Copy link
Author

This PR has been superseded by PR #1656. Close it.

@bitnitdit bitnitdit closed this Nov 21, 2017
@bitnitdit bitnitdit deleted the erl_opts/no_debug_info-not-working branch November 21, 2017 04:41
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.

2 participants