overrides on erl_opts does not work as expected

[RoadRunnr]

Current behaviour

Using a override to force to a specific erl_opts setting on all or a specific dependency completely nukes the existing erl_opts settings.

e.g.:

{{overrides, [{override, [{erl_opts, [native, {hipe, o3}]}]}]

will completely overwrite the erl_opts of all dependencies.

This becomes a problem if the dependency declares something important in it erl_opts, for example a parse_transform. Overriding erl_opts for every dependency separately to capture their original erl_opts quickly becomes a nightmare.

Expected behaviour

overrides should merge with existing settings or an explicit merge option should exists for overrides.

[ferd]

This works as expected. You just want to use the add form of overrides; these do not allow you to do it for all deps/apps at once, though.

[RoadRunnr]

mhh, that means that the example from the (documentation)[https://www.rebar3.org/docs/configuration#overrides]:

{overrides, [{override, [{erl_opts, [debug_info]}]}]}.

is pretty much useless as it would break any dependency application that needs additionl erl_opts.

Also, with add not working at the global level and without an way to remove options (like debug_info), this whole mechnisms is not usable for erl_opts....

[ferd]

It is usable for the erl_opts of specific deps at this point. It can be argued that we need add at a more global scope, but overrides are not broken as they are, and in fact work 100% as intended.

Overrides were initially meant more as a measure to ease migration from rebar 2.x to rebar3 so it has not been fledged past a point of getting most deps that were breaking to working states (i.e. changing deps, adding plugins, replacing opts, and so on). It's possible we need to improve their support overall, if that's a direction we want to go to.

You also have to ponder, for example, what would be the result of adding:

{overrides, [{add, [{overrides, [{override, []}]}]}.

To a compile target and so on.

There's possibly a reason why we didn't allow add to be applied to all deps, we would need to check the issue history for that. We possibly just felt it was already very complex as is (there's been about or more than a dozen issues about overrides and their semantics), but I don't recall for now. It's possible we especially left it that way to discourage using it for more than transitioning instead of as a global tool to manipulate full dep trees with. Any idea @tsloughter ?

[talentdeficit]

for erl_opts specifically it's because there's no general merge algorithm

consider:

[{d, foo}], [{d, foo, false}], [{d, foo, "foo"}]

how would you merge these three erl_opts arguments?

[RoadRunnr]

Found another example where overrides on erl_opts completely break the functionality of dependency.

jsx uses a rebar config script to set some defines for enabling maps support. Attempting to add a compile time option (like native) through to an override nukes that detection logic.

[{d, foo}], [{d, foo, false}], [{d, foo, "foo"}]

how would you merge these three erl_opts arguments?

Simple, you don't. This is a simple case of the last option wins. Last meaning here the top most define in the dependency hierarchy.

However, this logic might me applicable to defines, but not to all compile options, some need aditive merge (e.g. parse_transform) others might need mechanisms to remove the (e.g. debug_info).

So this feels like a per option merging strategy is needed for erl_opts.

Whatever the solution, we need a way to force some compile time options for dependencies to certain values without touching other options set by that dependency. The most important ones being: debug_info, native, hipe and d (with merging). Optionally I can see this being useful for: encrypt_debug_info and parse_transform

[talentdeficit]

doesn't add work for that jsx case?

we probably do need a way to set debug_info, encrypt_debug_info, native, hipe and defines independent of overrides however. i think probably it's cleaner and more useable to do it outside of erl_opts however. if you open an issue for that i can spend some time on it

[plux]

I made a PR that should enable add to work globally, like override can.

#1396

[ferd]

I believe this is fixed by a combination of #1759, #1798, #1656 and #1606