[erlang-patches] The 'start_phases' entry in .app files is incorrectly set to 'undefined' when generating a release
Siri Hansen
erlangsiri@REDACTED
Thu Jan 12 09:58:42 CET 2012
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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://erlang.org/pipermail/erlang-patches/attachments/20120112/1d51db42/attachment.htm>
More information about the erlang-patches
mailing list