[erlang-patches] The 'start_phases' entry in .app files is incorrectly set to 'undefined' when generating a release
Juan Jose Comellas
juanjo@REDACTED
Wed Jan 11 18:42:52 CET 2012
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