[erlang-patches] The 'start_phases' entry in .app files is incorrectly set to 'undefined' when generating a release

Wed Jan 11 11:45:42 CET 2012

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.gitjc-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/20120111/79c2dcfa/attachment.htm>