[erlang-patches] The 'start_phases' entry in .app files is incorrectly set to 'undefined' when generating a release
Juan Jose Comellas
juanjo@REDACTED
Thu Jan 12 20:55:58 CET 2012
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