[erlang-patches] The 'start_phases' entry in .app files is incorrectly set to 'undefined' when generating a release
Juan Jose Comellas
juanjo@REDACTED
Fri Jan 13 14:34:10 CET 2012
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
>> >>> >
>> >>> >
>> >>
>> >>
>
>
More information about the erlang-patches
mailing list