[erlang-patches] The 'start_phases' entry in .app files is incorrectly set to 'undefined' when generating a release
Henrik Nord
henrik@REDACTED
Tue Jan 17 12:07:55 CET 2012
jc-omit-undefined-start_phases-3
I have added this version to 'pu'
Thank you !
On 01/13/2012 02:34 PM, Juan Jose Comellas wrote:
> Siri, I agree with making it more readable. This being my first commit
> to Erlang/OTP I wanted to make everything as unobtrusive and
> predictable as possible. I've changed the patch as you requested and I
> made two versions of it in the following branches:
>
> 1) jc-omit-undefined-start_phases-2
>
> Using the style present in the original file for the case expression
> (i.e. case StartMod =:= undefined of true -> ....)
>
> Fetch with:
> git fetch git://github.com/jcomellas/otp.git jc-omit-undefined-start_phases-2
> Diff against 'maint':
> https://github.com/jcomellas/otp/compare/erlang:maint...jcomellas:jc-omit-undefined-start_phases-2
>
>
> 2) jc-omit-undefined-start_phases-3
>
> Using the more modern Erlang style for case expressions that Tuncer
> suggested in his email (i.e. case StartMod of undefined -> ...)
>
> Fetch with:
> git fetch git://github.com/jcomellas/otp.git jc-omit-undefined-start_phases-3
> Diff against 'maint':
> https://github.com/jcomellas/otp/compare/erlang:maint...jcomellas:jc-omit-undefined-start_phases-3
>
> I've also tested a release with both branches and the resulting .app
> files are exactly the same, as expected:
>
> e.g. With 'start_phases' unset in the original .app file:
>
> %% app generated at {2012,1,13} {10,15,39}
> {application,sasl,
> [{description,"SASL CXC 138 11"},
> {vsn,"2.2"},
> {id,[]},
> {modules,[alarm_handler,erlsrv,format_lib_supp,misc_supp,
> overload,rb,rb_format_supp,release_handler,
> release_handler_1,sasl,sasl_report,sasl_report_file_h,
> sasl_report_tty_h,si,si_sasl_supp,systools,
> systools_lib,systools_make,systools_rc,
> systools_relup]},
> {registered,[sasl_sup,alarm_handler,overload,release_handler]},
> {applications,[kernel,stdlib]},
> {included_applications,[]},
> {env,[{sasl_error_logger,tty},{errlog_type,all}]},
> {maxT,infinity},
> {maxP,infinity},
> {mod,{sasl,[]}}]}.
>
> e.g. With 'start_phases' set to [] in the original .app file:
>
> %% app generated at {2012,1,13} {10,15,39}
> {application,etest,
> [{description,[]},
> {vsn,"2"},
> {id,[]},
> {modules,[etest_app,etest_sup]},
> {registered,[]},
> {applications,[kernel,stdlib]},
> {included_applications,[]},
> {env,[]},
> {maxT,infinity},
> {maxP,infinity},
> {mod,{etest_app,[]}},
> {start_phases,[]}]}.
>
> Let me know if any other change is required.
>
> Juanjo
>
>
> On Fri, Jan 13, 2012 at 6:55 AM, Siri Hansen<erlangsiri@REDACTED> wrote:
>> Hi Juan!
>>
>> Thanks for the patch - I had a look at your changes, and I have one comment:
>>
>> There is no requirement of any specific order of the items in the .app file
>> and I therefore think it would be more readable to just add the start_phases
>> and mod items at the end of the list instead of merging them with the maxT
>> and maxP items.
>>
>> I know that this change would make the result from this function differ
>> compared to earlier but it would still be correct, and I think that the
>> readability is quite important. What do you think?
>>
>> Regards
>> /siri
>>
>>
>> 2012/1/12 Juan Jose Comellas<juanjo@REDACTED>
>>> Siri, I have compiled the maint branch with my change and tested
>>> generating a release using rebar. The .app file I sent as an example
>>> before now looks like this:
>>>
>>> %% app generated at {2012,1,12} {16,49,25}
>>> {application,sasl,
>>> [{description,"SASL CXC 138 11"},
>>> {vsn,"2.2"},
>>> {id,[]},
>>> {modules,[alarm_handler,erlsrv,format_lib_supp,misc_supp,
>>> overload,rb,rb_format_supp,release_handler,
>>>
>>> release_handler_1,sasl,sasl_report,sasl_report_file_h,
>>> sasl_report_tty_h,si,si_sasl_supp,systools,
>>> systools_lib,systools_make,systools_rc,
>>> systools_relup]},
>>>
>>> {registered,[sasl_sup,alarm_handler,overload,release_handler]},
>>> {applications,[kernel,stdlib]},
>>> {included_applications,[]},
>>> {env,[{sasl_error_logger,tty},{errlog_type,all}]},
>>> {maxT,infinity},
>>> {maxP,infinity},
>>> {mod,{sasl,[]}}]}.
>>>
>>> You can see that now the .app file does not include the 'start_phases'
>>> entry. You can get this patch by running:
>>>
>>> git fetch git://github.com/jcomellas/otp.git
>>> jc-omit-undefined-start_phases
>>>
>>> Juanjo
>>>
>>>
>>> On Thu, Jan 12, 2012 at 4:30 PM, Juan Jose Comellas<juanjo@REDACTED>
>>> wrote:
>>>> Siri, I have made the change. You can review it here:
>>>>
>>>>
>>>> https://github.com/jcomellas/otp/compare/erlang:maint...jcomellas:jc-omit-undefined-start_phases
>>>>
>>>> And you can fetch the changes with:
>>>>
>>>> git fetch git://github.com/jcomellas/otp.git
>>>> jc-omit-undefined-start_phases
>>>>
>>>> I am recompiling the maint branch with this change and I intend to
>>>> test generating a release upgrade with it, but it's going to take a
>>>> while.
>>>>
>>>> Juanjo
>>>>
>>>>
>>>> On Thu, Jan 12, 2012 at 5:58 AM, Siri Hansen<erlangsiri@REDACTED>
>>>> wrote:
>>>>> Ok, thanks!
>>>>> Then I think that we will go for the solution where
>>>>> reltool_target:gen_app
>>>>> does not add the start_phases entry if
>>>>> #app_info.start_phases==undefined.
>>>>> And the default value for #app_info.start_phases shall still be
>>>>> 'undefined'.
>>>>> This will keep the backwards compatibility.
>>>>>
>>>>> Will you do the change, Juan?
>>>>>
>>>>> Regards
>>>>> /siri
>>>>>
>>>>>
>>>>> 2012/1/11 Juan Jose Comellas<juanjo@REDACTED>
>>>>>> Siri, what you mention is exactly what was happening. I did all my
>>>>>> tests with rebar and what I experienced was that after performing a
>>>>>> 'rebar generate' reltool creates all the .app files in the release
>>>>>> with 'start_phases' set as 'undefined'. Here's an example:
>>>>>>
>>>>>> %% app generated at {2012,1,11} {14,17,10}
>>>>>> {application,sasl,
>>>>>> [{description,"SASL CXC 138 11"},
>>>>>> {vsn,"2.2"},
>>>>>> {id,[]},
>>>>>> {modules,[alarm_handler,erlsrv,format_lib_supp,misc_supp,
>>>>>> overload,rb,rb_format_supp,release_handler,
>>>>>>
>>>>>> release_handler_1,sasl,sasl_report,sasl_report_file_h,
>>>>>> sasl_report_tty_h,si,si_sasl_supp,systools,
>>>>>> systools_lib,systools_make,systools_rc,
>>>>>> systools_relup]},
>>>>>>
>>>>>> {registered,[sasl_sup,alarm_handler,overload,release_handler]},
>>>>>> {applications,[kernel,stdlib]},
>>>>>> {included_applications,[]},
>>>>>> {env,[{sasl_error_logger,tty},{errlog_type,all}]},
>>>>>> {start_phases,undefined},
>>>>>> {maxT,infinity},
>>>>>> {maxP,infinity},
>>>>>> {mod,{sasl,[]}}]}.
>>>>>>
>>>>>> The problem is that once you try to run 'rebar generate-upgrade' you
>>>>>> get all kinds of badmatch errors because the function that validates
>>>>>> the 'start_phases' entry in the generated .app files does not allow it
>>>>>> to be set to 'undefined'. Here's the relevant line:
>>>>>>
>>>>>>
>>>>>>
>>>>>> https://github.com/erlang/otp/blob/master/lib/sasl/src/systools_make.erl#L681
>>>>>>
>>>>>> My thought was to set the 'start_phases' field to [] by default in the
>>>>>> #app_info record (in lib/reltool/src/reltool.hrl) to avoid this
>>>>>> problem. What you proposed (not adding the 'start_phases' entry if it
>>>>>> is set to 'undefined') is equally valid. Also, another option would be
>>>>>> to allow 'undefined' to be a valid value in the line I referenced from
>>>>>> Github above.
>>>>>>
>>>>>> This seems to have been broken for over a year, and from what I can
>>>>>> see it was impossible to automatically generate releases and release
>>>>>> upgrades without triggering this error (at least with rebar).
>>>>>>
>>>>>> Juanjo
>>>>>>
>>>>>>
>>>>>> On Wed, Jan 11, 2012 at 7:45 AM, Siri Hansen<erlangsiri@REDACTED>
>>>>>> wrote:
>>>>>>> Sorry for the delay, but finally I have had a look at this....
>>>>>>>
>>>>>>> As far as I can see the empty list used as default
>>>>>>> for #application.start_phase in systools.hrl will *never* come out
>>>>>>> in a
>>>>>>> real
>>>>>>> instance of the #application record. The fields of this record gets
>>>>>>> its
>>>>>>> values in systools_make:parse_application/4 (from get_items/2 ->
>>>>>>> check_item/2) - which will give the start_phases field the value
>>>>>>> 'undefined'
>>>>>>> if there is no start_phases entry in the .app file. The only way
>>>>>>> start_phases can get the value [] here is if there is a
>>>>>>> {start_phases,[]}
>>>>>>> entry in the .app file, but this has nothing to do with the default
>>>>>>> value.
>>>>>>>
>>>>>>> I agree of course that systools.hrl is a bit misleading by stating a
>>>>>>> default
>>>>>>> value which is never used...
>>>>>>>
>>>>>>> reltool_server on the other hand does use the default value from
>>>>>>> reltool.hrl
>>>>>>> file when parsing - but (and this might be the real bug)
>>>>>>> reltool_target
>>>>>>> does
>>>>>>> the opposite operation - i.e. it takes information from #app_info
>>>>>>> records
>>>>>>> and generates app specifications where it takes the value of the
>>>>>>> start_phases field in the record and inserts it directly as a
>>>>>>> {start_phases,Phases} entry in the app specification. It then seems
>>>>>>> like
>>>>>>> this is written to a .app file during target installation. I am not
>>>>>>> 100%
>>>>>>> sure of this (and why), so any objections would be good... But
>>>>>>> anyway,
>>>>>>> if
>>>>>>> this is the case - there might be .app files containing
>>>>>>> {start_phases,undefined} in the target installation from reltool...
>>>>>>> And
>>>>>>> this
>>>>>>> can never be parsed by systools!!!
>>>>>>>
>>>>>>> Could this be what happens in your case, Juan?
>>>>>>>
>>>>>>> If so, I think the best correction of this would be to let
>>>>>>> reltool_target
>>>>>>> insert a {start_phases,Phases} entry in the app specification only
>>>>>>> if
>>>>>>> Phases=/=undefined... What do you think?
>>>>>>>
>>>>>>> Regards
>>>>>>> /siri
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 2012/1/5 Ulf Wiger<ulf@REDACTED>
>>>>>>>>
>>>>>>>> I would suggest that the special handling of {start_phases,
>>>>>>>> undefined}
>>>>>>>> be
>>>>>>>> removed, and documentation updated accordingly.
>>>>>>>>
>>>>>>>> But this is a decision that OTP will have to make.
>>>>>>>>
>>>>>>>> BR,
>>>>>>>> Ulf W
>>>>>>>>
>>>>>>>> On 5 Jan 2012, at 16:04, Juan Jose Comellas wrote:
>>>>>>>>
>>>>>>>>> Would it be an option to modify the function that validates the
>>>>>>>>> value
>>>>>>>>> assigned to 'start_phases' so that 'undefined' is considered an
>>>>>>>>> acceptable value? The relevant line is:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> https://github.com/erlang/otp/blob/master/lib/sasl/src/systools_make.erl#L681
>>>>>>>>>
>>>>>>>>> Juanjo
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Jan 3, 2012 at 5:31 PM, Ulf Wiger<ulf@REDACTED>
>>>>>>>>> wrote:
>>>>>>>>>> Not my call.
>>>>>>>>>>
>>>>>>>>>> I can well understand if the reasons behind the special
>>>>>>>>>> treatment of
>>>>>>>>>> 'start_phases' have been forgotten by those maintaining the
>>>>>>>>>> code.
>>>>>>>>>>
>>>>>>>>>> A closer analysis might reveal whether backward compatibility
>>>>>>>>>> has
>>>>>>>>>> already been sufficiently compromised that it is now time to
>>>>>>>>>> Make It
>>>>>>>>>> Right
>>>>>>>>>> (™).
>>>>>>>>>>
>>>>>>>>>> Even the old AXD 301 project, for which the oddity was
>>>>>>>>>> introduced in
>>>>>>>>>> the first place, took the first opportunity to make the code
>>>>>>>>>> fully
>>>>>>>>>> compatible with the new semantics.
>>>>>>>>>>
>>>>>>>>>> BR,
>>>>>>>>>> Ulf W
>>>>>>>>>>
>>>>>>>>>> On 3 Jan 2012, at 21:22, Juan Jose Comellas wrote:
>>>>>>>>>>
>>>>>>>>>>> I can make the change and add it to the branch I created, but
>>>>>>>>>>> the
>>>>>>>>>>> question is: should I? I'm not really that well informed about
>>>>>>>>>>> the
>>>>>>>>>>> history of this value. All I can say is that the parts that
>>>>>>>>>>> care
>>>>>>>>>>> about
>>>>>>>>>>> the 'start_phases' entry in reltool and systools assume that it
>>>>>>>>>>> will
>>>>>>>>>>> be an empty list when it is not set and fail otherwise.
>>>>>>>>>>>
>>>>>>>>>>> Let me know if I have to modify my patch to make it acceptable
>>>>>>>>>>> for
>>>>>>>>>>> inclusion in OTP.
>>>>>>>>>>>
>>>>>>>>>>> Juanjo
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jan 3, 2012 at 8:22 AM, Ulf Wiger<ulf@REDACTED>
>>>>>>>>>>> wrote:
>>>>>>>>>>>> Hmm, there's an old catch here. Someone will have to hollar if
>>>>>>>>>>>> it
>>>>>>>>>>>> is
>>>>>>>>>>>> no
>>>>>>>>>>>> longer relevant - but if it isn't, some documentation patch is
>>>>>>>>>>>> called
>>>>>>>>>>>> for.
>>>>>>>>>>>>
>>>>>>>>>>>> In
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> http://www.erlang.org/doc/apps/kernel/application.html#Module:start-2
>>>>>>>>>>>>
>>>>>>>>>>>> StartType defines the type of start:
>>>>>>>>>>>>
>>>>>>>>>>>> normal if it's a normal startup.
>>>>>>>>>>>> normal also if the application is distributed and started at
>>>>>>>>>>>> the
>>>>>>>>>>>> current
>>>>>>>>>>>> node due to a failover from another node, and the application
>>>>>>>>>>>> specification
>>>>>>>>>>>> key start_phases == undefined.
>>>>>>>>>>>> {takeover,Node} if the application is distributed and started
>>>>>>>>>>>> at
>>>>>>>>>>>> the
>>>>>>>>>>>> current
>>>>>>>>>>>> node due to a takeover from Node, either because
>>>>>>>>>>>> application:takeover/2 has
>>>>>>>>>>>> been called or because the current node has higher priority
>>>>>>>>>>>> than
>>>>>>>>>>>> Node.
>>>>>>>>>>>> {failover,Node} if the application is distributed and started
>>>>>>>>>>>> at
>>>>>>>>>>>> the
>>>>>>>>>>>> current
>>>>>>>>>>>> node due to a failover from Node, and the application
>>>>>>>>>>>> specification
>>>>>>>>>>>> key start_phases /= undefined.
>>>>>>>>>>>>
>>>>>>>>>>>> Note that StartType = {failover, Node} is only used if
>>>>>>>>>>>> start_phases
>>>>>>>>>>>> /=
>>>>>>>>>>>> undefined. I will guess that it is for this reason that
>>>>>>>>>>>> start_phases
>>>>>>>>>>>> is
>>>>>>>>>>>> actually set to undefined as default. If it isn't, it was a
>>>>>>>>>>>> happy
>>>>>>>>>>>> coincidence, since it actually preserves backward
>>>>>>>>>>>> compatibility.
>>>>>>>>>>>>
>>>>>>>>>>>> This particular oddity was introduced many years ago, in the
>>>>>>>>>>>> 90s -
>>>>>>>>>>>> possibly
>>>>>>>>>>>> even before Erlang was released as Open Source, so the legacy
>>>>>>>>>>>> argument may
>>>>>>>>>>>> not be that relevant.
>>>>>>>>>>>>
>>>>>>>>>>>> OTOH, I don't think many people use the start_phases
>>>>>>>>>>>> attribute, so
>>>>>>>>>>>> even new
>>>>>>>>>>>> code might break if the Mod:start/2 function is suddenly
>>>>>>>>>>>> called
>>>>>>>>>>>> with
>>>>>>>>>>>> a
>>>>>>>>>>>> StartType that was never seen before.
>>>>>>>>>>>>
>>>>>>>>>>>> BR,
>>>>>>>>>>>> Ulf W
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 3 Jan 2012, at 05:21, Juan Jose Comellas wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> The default value for 'start_phases' in .app files should be
>>>>>>>>>>>> [],
>>>>>>>>>>>> but
>>>>>>>>>>>> it is incorrectly set to 'undefined' when generating a
>>>>>>>>>>>> release.
>>>>>>>>>>>> This
>>>>>>>>>>>> happens when the original .app file does not set a value for
>>>>>>>>>>>> 'start_phases' explicitly.
>>>>>>>>>>>>
>>>>>>>>>>>> The reltool application defines its own copy of a record to
>>>>>>>>>>>> handle
>>>>>>>>>>>> .app files (#app_info{}, defined in
>>>>>>>>>>>> lib/reltool/src/reltool.hrl)
>>>>>>>>>>>> that
>>>>>>>>>>>> has different default values than the one used by systools
>>>>>>>>>>>> (#application{}, defined in lib/sasl/src/systools.hrl). In
>>>>>>>>>>>> particular,
>>>>>>>>>>>> the 'start_phases' entry is assumed by all of OTP to be []
>>>>>>>>>>>> when it
>>>>>>>>>>>> is
>>>>>>>>>>>> not explicitly set but reltool sets it to 'undefined' by
>>>>>>>>>>>> default.
>>>>>>>>>>>> This
>>>>>>>>>>>> causes badmatch errors when trying to generate release
>>>>>>>>>>>> upgrades.
>>>>>>>>>>>>
>>>>>>>>>>>> Without this patch, all of the rebar examples that generate
>>>>>>>>>>>> release
>>>>>>>>>>>> upgrades that I've found on the web will fail. The bug was
>>>>>>>>>>>> introduced
>>>>>>>>>>>> in R14A.
>>>>>>>>>>>>
>>>>>>>>>>>> git fetch git://github.com/jcomellas/otp.git
>>>>>>>>>>>> jc-fix-default-start-phases
>>>>>>>>>>>>
>>>>>>>>>>>> Juanjo
>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>> erlang-patches mailing list
>>>>>>>>>>>> erlang-patches@REDACTED
>>>>>>>>>>>> http://erlang.org/mailman/listinfo/erlang-patches
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> erlang-patches mailing list
>>>>>>>> erlang-patches@REDACTED
>>>>>>>> http://erlang.org/mailman/listinfo/erlang-patches
>>>>>>>
>>>>>
>>
> _______________________________________________
> erlang-patches mailing list
> erlang-patches@REDACTED
> http://erlang.org/mailman/listinfo/erlang-patches
--
/Henrik Nord Erlang/OTP
More information about the erlang-patches
mailing list