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

Make debug_info rules clear #1656

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Make debug_info rules clear #1656

merged 1 commit into from
Nov 21, 2017

Conversation

ferd
Copy link
Collaborator

@ferd ferd commented Nov 6, 2017

Current rebar3 uses debug_info rules where debug_info is added by
default, and no_debug_info prevents the default from being added, and
removes any explicit debug_info, if any. The problem is that if
'no_debug_info' is anywhere in the config for a run, it cannot be
removed even with other profiles. additionally, no_debug_info ignores
special tuples like {debug_info, {Mod, Data}} and {debug_info_key, Key},
which can be used to add debug info and encrypt it (in lieu of plain
debug_info) respectively.

This patch makes it so that the following rules are in place:

  • the first option seen takes priority, allowing profile overrides by
    the ordering rules
  • because the compiler apparently does not care for that order (it does
    not have to), the overriden options shall be explicitly deleted
  • any option related to debug info seen first cancels any no_debug_info
    that follows
  • any no_debug_info option seen first cancels all of the other
    debug_info options
  • if debug_info is seen first, it cancels out {debug_info_key, Key}
  • if {debug_info_key, Key} is seen first, it cancels out debug_info
  • All other options are left untouched in that context (defines can
    still be expanded and so on)

This should allow proper profile rules to be followed. See #1653 for context.

@ferd
Copy link
Collaborator Author

ferd commented Nov 6, 2017

Interesting. I still get the failures in CI. Will have to also investigate that one further.

@bitnitdit
Copy link

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

@ferd
Copy link
Collaborator Author

ferd commented Nov 20, 2017

I found the problem; erl_opts are specifically merged in the profile-order so that if you go rebar3 as a,b,c compile, the options from profile A come before B, which come before C, because that's how the compiler handles them. My PR right here assumed they were being merged like other keys (C > B > A) to give proplist preference.

I'll rework the PR so it's done on the right direction.

Current rebar3 uses debug_info rules where debug_info is added by
default, and no_debug_info prevents the default from being added, and
removes any explicit debug_info, if any. The problem is that if
'no_debug_info' is anywhere in the config for a run, it cannot be
removed even with other profiles. additionally, no_debug_info ignores
special tuples like {debug_info, {Mod, Data}} and {debug_info_key, Key},
which can be used to add debug info and encrypt it (in lieu of plain
debug_info) respectively.

This patch makes it so that the following rules are in place:

- the last option seen takes priority, allowing profile overrides by
  the ordering rules the compiler expects
- the overriden options shall be explicitly deleted to avoid confusing
  the compiler
- any option related to debug info seen last cancels any no_debug_info
  that preceded it
- any no_debug_info option seen last cancels all of the other
  debug_info options
- if debug_info is seen last, it cancels out {debug_info_key, Key}
- if {debug_info_key, Key} is seen last, it cancels out debug_info
- All other options are left untouched in that context (defines can
  still be expanded and so on)

This should allow proper profile rules to be followed. Note that erl_opt
profile merging puts precedence on the *last* element of a list, to
match the compiler.
@ferd ferd force-pushed the unconfuse-debug_info branch from e3f2b3b to bf7ec38 Compare November 20, 2017 19:58
@ferd ferd merged commit 2792029 into erlang:master Nov 21, 2017
ferd added a commit to ferd/rebar3 that referenced this pull request Apr 14, 2018
We got funny interactions since PR erlang#1656:

- the last `debug_info`-related option seen in a list of options after
  profile merge is kept, allowing later profiles from overtaking earlier
  ones
- if you go `rebar3 as a,b,c compile`, the options from profile A come
  before B, which come before C, so C's options win
- overrides are applied in order of profile as well, giving a priority
  to a later profile than an earlier one
- The values in overrides are prepended rather than suffixed to the
  existing list
- this means if we have to overrides adding options, such as `default`
  adding `no_debug_info', and `dialyze` adding `debug_info`, the
  results are `[debug_info]` as `dialyze` is applied first, and then
  `[no_debug_info, debug_info]` as `default` overrides are applied
- the final result is `no_debug_info` always winning when erl_opts are
  overriden specifically.

only `debug_info` options are going to suffer this, and in the context of
overrides. Other `erl_opts` should be fine. I'm not sure how that can be
fixed at all.

In the meanwhile, we can add support back while leaving the default to
not having debug information. This is done by:

- moving all `no_debug_info` options to the `prod` profile
- forcing `prod` to be called by `./bootstrap` so that most people keep
  getting no debug info
- anyone calling `rebar3 clean -a`  and then rebuilding with `rebar3
  escriptize` (i.e. rebar3 devs) get debug info going

This is up for review and discussion.
uwiger pushed a commit to aeternity/rebar3 that referenced this pull request Nov 27, 2018
We got funny interactions since PR erlang#1656:

- the last `debug_info`-related option seen in a list of options after
  profile merge is kept, allowing later profiles from overtaking earlier
  ones
- if you go `rebar3 as a,b,c compile`, the options from profile A come
  before B, which come before C, so C's options win
- overrides are applied in order of profile as well, giving a priority
  to a later profile than an earlier one
- The values in overrides are prepended rather than suffixed to the
  existing list
- this means if we have to overrides adding options, such as `default`
  adding `no_debug_info', and `dialyze` adding `debug_info`, the
  results are `[debug_info]` as `dialyze` is applied first, and then
  `[no_debug_info, debug_info]` as `default` overrides are applied
- the final result is `no_debug_info` always winning when erl_opts are
  overriden specifically.

only `debug_info` options are going to suffer this, and in the context of
overrides. Other `erl_opts` should be fine. I'm not sure how that can be
fixed at all.

In the meanwhile, we can add support back while leaving the default to
not having debug information. This is done by:

- moving all `no_debug_info` options to the `prod` profile
- forcing `prod` to be called by `./bootstrap` so that most people keep
  getting no debug info
- anyone calling `rebar3 clean -a`  and then rebuilding with `rebar3
  escriptize` (i.e. rebar3 devs) get debug info going

This is up for review and discussion.
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.

3 participants