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.

Sign up for GitHub

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

Jump to bottom

fix: have some sys configs with one -config flag #2071

Closed
wants to merge 2 commits into from
Closed

fix: have some sys configs with one -config flag #2071

wants to merge 2 commits into from

Conversation

pouriya
Copy link
Contributor

@pouriya pouriya commented Dec 23, 2018 

~ $ echo []. > sys.config
~ $ echo []. > sys2.config
~ $ erl -config sys.config sys2.config 
2018-12-23 23:42:00.004390 Error in process ~p with exit value:~n~p~n
	<0.42.0>
	{{case_clause,{'EXIT',{function_clause,[{application_controller,'-check_conf/0-fun-0-',[["sys.config","sys2.config"],[]],[{file,"application_controller.erl"},{line,1804}]},{lists,foldl,3,[{file,"lists.erl"},{line,1263}]},{application_controller,check_conf,0,[{file,"application_controller.erl"},{line,1803}]},{application_controller,init,2,[{file,"application_controller.erl"},{line,486}]}]}}},[{application_controller,init,2,[{file,"application_controller.erl"},{line,486}]}]}
{"could not start kernel pid",application_controller,"{{case_clause,{'EXIT',{function_clause,[{application_controller,'-check_conf/0-fun-0-',[[\"sys.config\",\"sys2.config\"],[]],[{file,\"application_controller.erl\"},{line,1804}]},{lists,foldl,3,[{file,\"lists.erl\"},{line,1263}]},{application_controller,check_conf,0,[{file,\"application_controller.erl\"},{line,1803}]},{application_controller,init,2,[{file,\"application_controller.erl\"},{line,486}]}]}}},[{application_controller,init,2,[{file,\"application_controller.erl\"},{line,486}]}]}"}
could not start kernel pid (application_controller) ({{case_clause,{'EXIT',{function_clause,[{application_controller,'-check_conf/0-fun-0-',[["sys.config","sys2.config"],[]],[{file,"application_contro

Crash dump is being written to: erl_crash.dump...done

Because here we have [["sys.config", "sys2.config"]] and here it does not be matched. So we have to run:

~ $ erl -config sys.config -config sys2.config 
Erlang/OTP 21 [erts-10.1] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:1] [hipe]
Eshell V10.1  (abort with ^G)
1>

I was adding dynamic configuration based changes in sys config files and I found this. Sorry if branch name does not relate to this.

@crownedgrouse
Copy link
Contributor

Hi,
Why there is a need of several top config files when it is possible to point to other config files in a single one...

@pouriya
Copy link
Contributor Author

pouriya commented Dec 25, 2018

Current implementation reads all files. The difference is that it works with -config sys1.config -config sys2.config, But does not work with -config sys1.config sys2.config.
Also I think it's useful for large systems. There is no need to have sys.config. We can just have app1.config, app2.config, appN.config.

@crownedgrouse
Copy link
Contributor

sys.config has a special meaning in release. Only this file can point to other includes. As far I can remember.

@crownedgrouse
Copy link
Contributor

'sys.config
When starting Erlang in embedded mode, it is assumed that exactly one system configuration file is used, named sys.config. This file is to be located in $ROOT/releases/Vsn, where $ROOT is the Erlang/OTP root installation directory and Vsn is the release version.

Release handling relies on this assumption. When installing a new release version, the new sys.config is read and used to update the application configurations'

josevalim
josevalim reviewed Dec 27, 2018
View reviewed changes
lib/kernel/src/application_controller.erl Outdated
@@ -1833,7 +1833,7 @@ check_conf() ->
{error, {Line, _Mod, Str}} ->
throw({error, {FName, Line, Str}})
end
end, [], Files)};
end, [], lists:foldr(fun erlang:'++'/2, [], Files))};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is equivalent to lists:flatten/1?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather to lists:append, since it will only go 1 level deep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I tested flatten/1 before:

$ erl -foo a b c -foo x y z
Erlang/OTP 20 [erts-9.2] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [kernel-poll:false]

Eshell V9.2  (abort with ^G)
1> {_, V} = init:get_argument(foo).
{ok,[["a","b","c"],["x","y","z"]]}

2> lists:flatten(V).
"abcxyz"

3> lists:foldr(fun erlang:'++'/2, [], V).
["a","b","c","x","y","z"]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point, thanks. In this case, lists:append/1 proposed by @ferd should do it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

append/1 works well. Thank you @ferd .

@uabboli uabboli added the team:MW label Jan 3, 2019
@sirihansen sirihansen self-assigned this Jan 16, 2019
@sirihansen
Copy link
Contributor

I think this change is ok. Even if release handling assumes one config file only, it is already possible to use several config files when release handling is not an issue. So this change does not open any door that isn't already open.

The PR does however need some tests, and the documentation of the -config option to erl must be updated - it currently says "Specifies the name of a configuration file...".

@sirihansen sirihansen added the waiting waiting for changes/input from author label Jan 16, 2019
@uabboli uabboli added team:VM Assigned to OTP team VM and removed team:MW labels Aug 21, 2019
@garazdawi
Copy link
Contributor

Hello @pouriya-jahanbakhsh do you plan to add tests+docs? or should I close this PR?

@pouriya
Copy link
Contributor Author

pouriya commented Aug 24, 2019

Hi. I'm sorry for the delay in response. I'm working on it.

@pouriya pouriya mentioned this pull request Aug 24, 2019
Closed
@pouriya
Copy link
Contributor Author

pouriya commented Aug 24, 2019

Bad news is that I deleted my fork of OTP in github and my PC :-S. So I've opened a new pr #2359.

@pouriya pouriya closed this Aug 24, 2019
@pouriya pouriya mentioned this pull request Sep 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josevalim josevalim josevalim left review comments

@ferd ferd ferd left review comments

Assignees
No one assigned
Labels
team:VM Assigned to OTP team VM waiting waiting for changes/input from author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pouriya @crownedgrouse @sirihansen @garazdawi @josevalim @ferd @uabboli