[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:30:35 CET 2012
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