Fix recursive profile merging, particularly for umbrella apps #1606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a config file exists at the root of a project, defines a given configuration value for a given profile, and that a sub-application (umbrella app) also has the same profile defined with the same key (but different values), the configuration values of the sub-application's profile would get silently dropped. The problem being that when the function to merge profiles is applied recursively, it is applied to each profile (so it will merge on the keys test, prod, etc.) rather than to each of the values of each profile. This patch reworks the profile merging so that the current behaviour is respected overall (a profile cannot be cancelled by a subdep's non-existant profile since its value should have been ignored), but ensures that sub-deps' profiles are otherwise applied recursively with the proper rules: - dependencies favor prior values - plugins favor new values - erl_first_files combine the lists - relx uses the tuple merge algorithm - erl_opts has its own custom merge as well - otherwise the new value takes precedence A test has also been added. There is a risk of breakage in some applications that may have relied on the buggy behaviour to work, though at this time we are aware of none of them.
ferd
commented
Aug 11, 2017
| true -> []; | ||
| false -> [{d, 'TEST'}] | ||
| true -> Opts; | ||
| false -> [{d, 'TEST'}|Opts] |
There was a problem hiding this comment.
not sure this changes anything in practice, but felt like a good gotcha
tsloughter
approved these changes
Aug 13, 2017
src/rebar_opts.erl
Outdated
| @@ -117,7 +117,10 @@ merge_opt(plugins, NewValue, _OldValue) -> | |||
| merge_opt({plugins, _}, NewValue, _OldValue) -> | |||
| NewValue; | |||
| merge_opt(profiles, NewValue, OldValue) -> | |||
| dict:to_list(merge_opts(dict:from_list(NewValue), dict:from_list(OldValue))); | |||
| ToMerge = fill_profile_gaps(lists:sort(NewValue), | |||
There was a problem hiding this comment.
Can you add a comment on what fill_profile_gaps is doing
There was a problem hiding this comment.
Done; renamed, add type specs and comments.
|
I don't think we have much to worry about with regards to projects that relied on the buggy behaviour. Though I could be wrong about how many projects actually use sub-apps and sub rebar.configs, but I don't think it is that many. |
This was referenced Aug 13, 2017
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a config file exists at the root of a project, defines a given
configuration value for a given profile, and that a sub-application
(umbrella app) also has the same profile defined with the same key (but
different values), the configuration values of the sub-application's
profile would get silently dropped.
The problem being that when the function to merge profiles is applied
recursively, it is applied to each profile (so it will merge on the keys
test, prod, etc.) rather than to each of the values of each profile.
This patch reworks the profile merging so that the current behaviour is
respected overall (a profile cannot be cancelled by a subdep's
non-existant profile since its value should have been ignored), but
ensures that sub-deps' profiles are otherwise applied recursively with
the proper rules:
A test has also been added.
There is a risk of breakage in some applications that may have relied on
the buggy behaviour to work, though at this time we are aware of none of
them.
This fixes #1566, and possibly #1247, #1477 and #1368
This is kind of a scary change, awaiting reviews before pushing forwards.