[erlang-patches] The 'start_phases' entry in .app files is incorrectly set to 'undefined' when generating a release
Siri Hansen
erlangsiri@REDACTED
Tue Jan 17 12:07:42 CET 2012
Juan - I think we will go for solution 2) (i.e.
jc-omit-undefined-start_phases-3). Thanks! I've asked Henrik to include it
in pu.
Regards
/siri
2012/1/13 Juan Jose Comellas <juanjo@REDACTED>
> Siri, I agree with making it more readable. This being my first commit
> to Erlang/OTP I wanted to make everything as unobtrusive and
> predictable as possible. I've changed the patch as you requested and I
> made two versions of it in the following branches:
>
> 1) jc-omit-undefined-start_phases-2
>
> Using the style present in the original file for the case expression
> (i.e. case StartMod =:= undefined of true -> ....)
>
> Fetch with:
> git fetch git://github.com/jcomellas/otp.gitjc-omit-undefined-start_phases-2
> Diff against 'maint':
>
> https://github.com/jcomellas/otp/compare/erlang:maint...jcomellas:jc-omit-undefined-start_phases-2
>
>
> 2) jc-omit-undefined-start_phases-3
>
> Using the more modern Erlang style for case expressions that Tuncer
> suggested in his email (i.e. case StartMod of undefined -> ...)
>
> Fetch with:
> git fetch git://github.com/jcomellas/otp.gitjc-omit-undefined-start_phases-3
> Diff against 'maint':
>
> https://github.com/jcomellas/otp/compare/erlang:maint...jcomellas:jc-omit-undefined-start_phases-3
>
> I've also tested a release with both branches and the resulting .app
> files are exactly the same, as expected:
>
> e.g. With 'start_phases' unset in the original .app file:
>
> %% app generated at {2012,1,13} {10,15,39}
> {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,[]}}]}.
>
> e.g. With 'start_phases' set to [] in the original .app file:
>
> %% app generated at {2012,1,13} {10,15,39}
> {application,etest,
> [{description,[]},
> {vsn,"2"},
> {id,[]},
> {modules,[etest_app,etest_sup]},
> {registered,[]},
> {applications,[kernel,stdlib]},
> {included_applications,[]},
> {env,[]},
> {maxT,infinity},
> {maxP,infinity},
> {mod,{etest_app,[]}},
> {start_phases,[]}]}.
>
> Let me know if any other change is required.
>
> Juanjo
>
>
> On Fri, Jan 13, 2012 at 6:55 AM, Siri Hansen <erlangsiri@REDACTED> wrote:
> > Hi Juan!
> >
> > Thanks for the patch - I had a look at your changes, and I have one
> comment:
> >
> > There is no requirement of any specific order of the items in the .app
> file
> > and I therefore think it would be more readable to just add the
> start_phases
> > and mod items at the end of the list instead of merging them with the
> maxT
> > and maxP items.
> >
> > I know that this change would make the result from this function differ
> > compared to earlier but it would still be correct, and I think that the
> > readability is quite important. What do you think?
> >
> > Regards
> > /siri
> >
> >
> > 2012/1/12 Juan Jose Comellas <juanjo@REDACTED>
> >>
> >> 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
> >> >>> >
> >> >>> >
> >> >>
> >> >>
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://erlang.org/pipermail/erlang-patches/attachments/20120117/fbed81b8/attachment.htm>
More information about the erlang-patches
mailing list