Do not discard parameters --system_libs and --include-erts #2503
Conversation
|
I'm trying to verify that we actually drop all arguments. Feel like a major thing to miss, haha, but I wouldn't put it passed me... If that is the case then we need to add support for every option you see in I wouldn't normally say you need to add support for all of those to get this merged, but I think what you are doing here should be done with a generic function for merging in any command line opt. I'd probably accept it even if it only actually was used for system libs and include erts at first. |
|
I think the arguments from command line should take the preference here. For my use cases I only use system_lib and include_erts, but others are equally important. Example: currently vm_args and sys_config are also not taking into consideration. Let me try to rework my PR and fix the other parameters, like you said with a generic function. |
1eafb44 to
9dbc00b
Compare
|
I've implemented a generic approach for the following arguments: The overlay_vars argument needs to take in consideration before because, if present, it needs to be added to overlay_vars list. |
|
Hello @tsloughter let me know what else I can do in order to get this one merged. Thanks. |
src/rebar_relx.erl
Outdated
| V -> | ||
| lists:keystore(AliasOpt, 1, Acc, {AliasOpt, V}) | ||
| end; | ||
| (Opt, Acc) -> |
There was a problem hiding this comment.
Formatting, just need to indent this and the lines below it to match the args of the fun above it.
src/rebar_relx.erl
Outdated
|
|
||
| maybe_obey_command_args(RelxConfig, Opts, Args) -> | ||
| lists:foldl( | ||
| fun({Opt, AliasOpt}, Acc) -> |
There was a problem hiding this comment.
AliasOpt? Are you having vm_args replaced with vm_args_src?
They are separate args. Also, sys_config_src exists as an option but isn't included.
There was a problem hiding this comment.
yep, they should be mutually exclusive, rather than treating one as the priority of the other.
There was a problem hiding this comment.
Hmm alright. Now I got it. Right now, this PR is just forwarding the vm_args and sys_config to relx.
If I have a release configuration like this:
{relx, [
{release, {elock, "0.1.0"}, [elock, sasl],
[
{dev_mode, true},
{include_erts, false},
{system_libs, false},
{extended_start_script, true},
{generate_start_script, true},
{overlay, []}
]},
The following command will work: rebar3 tar --system_libs $REBAR3_TARGET_SYSTEM_LIBS --include-erts $REBAR3_TARGET_INCLUDE_ERTS -n elock --sys_config config/sys-test.config --vm_args config/vm-test.args.
On the other hand, the following release configuration will not work with the same rebar3 tar like the above command:
{relx, [
{release, {elock, "0.1.0"}, [elock, sasl],
[
{sys_config, "./config/sys.config"},
{vm_args, "./config/vm.args"},
{dev_mode, true},
{include_erts, false},
{system_libs, false},
{extended_start_script, true},
{generate_start_script, true},
{overlay, []}
]},
src/rebar_relx.erl
Outdated
| undefined -> | ||
| []; | ||
| OverlayVars -> | ||
| [OverlayVars] |
There was a problem hiding this comment.
is this right? The name OverlayVars implies the result is a list, which you put in another list. The callsite goes [{base_dir, rebar_dir:base_dir(State)} | overlay_vars(Opts)] which implies that if you have overlay vars [{a, b}, {c, d}] (or even just [a, b]), this function will return [[{a,b}, {c,d}]] (or [[a,b]]) and will always get a bad result at the callsite.
If what you get back isn't a list, then the variable name is at least misleading.
There was a problem hiding this comment.
I see. That was wrong. I've fixed that to return the overlay_vars which should point to 'Path to a file of overlay variables'.
The following command line arguments, if present, will take into consideration when making a release (i.e, when calling rebar3 release or rebar3 tar) * include_erts * system_libs * vm_args * sys_config * overlay_vars The others arguments from rebar_relx:opt_spec_list/0 are already get from command line arguments. Fixes 2496
9dbc00b to
8510c5c
Compare
If the parameters --system_libs and --include-erts from rebar3 tar or
release are passed, then they need to take into account when loading
relx configuration file.
This commits checkes if the parameters are passed. If yes, they will be
used to override any system_libs and/or include_erts.
Fixes 2496