Ok, thanks!<div>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.<div><div>And the default value for #app_info.start_phases shall still be 'undefined'.<br>
<div>This will keep the backwards compatibility.</div><div><br></div><div>Will you do the change, Juan?</div><div><br></div><div>Regards</div><div>/siri</div><div><br></div><div><br><div class="gmail_quote">2012/1/11 Juan Jose Comellas <span dir="ltr"><<a href="mailto:juanjo@comellas.org">juanjo@comellas.org</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Siri, what you mention is exactly what was happening. I did all my<br>
tests with rebar and what I experienced was that after performing a<br>
'rebar generate' reltool creates all the .app files in the release<br>
with 'start_phases' set as 'undefined'. Here's an example:<br>
<br>
%% app generated at {2012,1,11} {14,17,10}<br>
{application,sasl,<br>
[{description,"SASL CXC 138 11"},<br>
{vsn,"2.2"},<br>
{id,[]},<br>
{modules,[alarm_handler,erlsrv,format_lib_supp,misc_supp,<br>
overload,rb,rb_format_supp,release_handler,<br>
release_handler_1,sasl,sasl_report,sasl_report_file_h,<br>
sasl_report_tty_h,si,si_sasl_supp,systools,<br>
systools_lib,systools_make,systools_rc,<br>
systools_relup]},<br>
{registered,[sasl_sup,alarm_handler,overload,release_handler]},<br>
{applications,[kernel,stdlib]},<br>
{included_applications,[]},<br>
{env,[{sasl_error_logger,tty},{errlog_type,all}]},<br>
{start_phases,undefined},<br>
{maxT,infinity},<br>
{maxP,infinity},<br>
{mod,{sasl,[]}}]}.<br>
<br>
The problem is that once you try to run 'rebar generate-upgrade' you<br>
get all kinds of badmatch errors because the function that validates<br>
the 'start_phases' entry in the generated .app files does not allow it<br>
to be set to 'undefined'. Here's the relevant line:<br>
<br>
<a href="https://github.com/erlang/otp/blob/master/lib/sasl/src/systools_make.erl#L681" target="_blank">https://github.com/erlang/otp/blob/master/lib/sasl/src/systools_make.erl#L681</a><br>
<br>
My thought was to set the 'start_phases' field to [] by default in the<br>
#app_info record (in lib/reltool/src/reltool.hrl) to avoid this<br>
problem. What you proposed (not adding the 'start_phases' entry if it<br>
is set to 'undefined') is equally valid. Also, another option would be<br>
to allow 'undefined' to be a valid value in the line I referenced from<br>
Github above.<br>
<br>
This seems to have been broken for over a year, and from what I can<br>
see it was impossible to automatically generate releases and release<br>
upgrades without triggering this error (at least with rebar).<br>
<br>
Juanjo<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On Wed, Jan 11, 2012 at 7:45 AM, Siri Hansen <<a href="mailto:erlangsiri@gmail.com">erlangsiri@gmail.com</a>> wrote:<br>
> Sorry for the delay, but finally I have had a look at this....<br>
><br>
> As far as I can see the empty list used as default<br>
> for #application.start_phase in systools.hrl will *never* come out in a real<br>
> instance of the #application record. The fields of this record gets its<br>
> values in systools_make:parse_application/4 (from get_items/2 -><br>
> check_item/2) - which will give the start_phases field the value 'undefined'<br>
> if there is no start_phases entry in the .app file. The only way<br>
> start_phases can get the value [] here is if there is a {start_phases,[]}<br>
> entry in the .app file, but this has nothing to do with the default value.<br>
><br>
> I agree of course that systools.hrl is a bit misleading by stating a default<br>
> value which is never used...<br>
><br>
> reltool_server on the other hand does use the default value from reltool.hrl<br>
> file when parsing - but (and this might be the real bug) reltool_target does<br>
> the opposite operation - i.e. it takes information from #app_info records<br>
> and generates app specifications where it takes the value of the<br>
> start_phases field in the record and inserts it directly as a<br>
> {start_phases,Phases} entry in the app specification. It then seems like<br>
> this is written to a .app file during target installation. I am not 100%<br>
> sure of this (and why), so any objections would be good... But anyway, if<br>
> this is the case - there might be .app files containing<br>
> {start_phases,undefined} in the target installation from reltool... And this<br>
> can never be parsed by systools!!!<br>
><br>
> Could this be what happens in your case, Juan?<br>
><br>
> If so, I think the best correction of this would be to let reltool_target<br>
> insert a {start_phases,Phases} entry in the app specification only if<br>
> Phases=/=undefined... What do you think?<br>
><br>
> Regards<br>
> /siri<br>
><br>
><br>
><br>
> 2012/1/5 Ulf Wiger <<a href="mailto:ulf@feuerlabs.com">ulf@feuerlabs.com</a>><br>
>><br>
>><br>
>> I would suggest that the special handling of {start_phases, undefined} be<br>
>> removed, and documentation updated accordingly.<br>
>><br>
>> But this is a decision that OTP will have to make.<br>
>><br>
>> BR,<br>
>> Ulf W<br>
>><br>
>> On 5 Jan 2012, at 16:04, Juan Jose Comellas wrote:<br>
>><br>
>> > Would it be an option to modify the function that validates the value<br>
>> > assigned to 'start_phases' so that 'undefined' is considered an<br>
>> > acceptable value? The relevant line is:<br>
>> ><br>
>> ><br>
>> > <a href="https://github.com/erlang/otp/blob/master/lib/sasl/src/systools_make.erl#L681" target="_blank">https://github.com/erlang/otp/blob/master/lib/sasl/src/systools_make.erl#L681</a><br>
>> ><br>
>> > Juanjo<br>
>> ><br>
>> ><br>
>> > On Tue, Jan 3, 2012 at 5:31 PM, Ulf Wiger <<a href="mailto:ulf@feuerlabs.com">ulf@feuerlabs.com</a>> wrote:<br>
>> >><br>
>> >> Not my call.<br>
>> >><br>
>> >> I can well understand if the reasons behind the special treatment of<br>
>> >> 'start_phases' have been forgotten by those maintaining the code.<br>
>> >><br>
>> >> A closer analysis might reveal whether backward compatibility has<br>
>> >> already been sufficiently compromised that it is now time to Make It Right<br>
>> >> (™).<br>
>> >><br>
>> >> Even the old AXD 301 project, for which the oddity was introduced in<br>
>> >> the first place, took the first opportunity to make the code fully<br>
>> >> compatible with the new semantics.<br>
>> >><br>
>> >> BR,<br>
>> >> Ulf W<br>
>> >><br>
>> >> On 3 Jan 2012, at 21:22, Juan Jose Comellas wrote:<br>
>> >><br>
>> >>> I can make the change and add it to the branch I created, but the<br>
>> >>> question is: should I? I'm not really that well informed about the<br>
>> >>> history of this value. All I can say is that the parts that care about<br>
>> >>> the 'start_phases' entry in reltool and systools assume that it will<br>
>> >>> be an empty list when it is not set and fail otherwise.<br>
>> >>><br>
>> >>> Let me know if I have to modify my patch to make it acceptable for<br>
>> >>> inclusion in OTP.<br>
>> >>><br>
>> >>> Juanjo<br>
>> >>><br>
>> >>><br>
>> >>> On Tue, Jan 3, 2012 at 8:22 AM, Ulf Wiger <<a href="mailto:ulf@feuerlabs.com">ulf@feuerlabs.com</a>> wrote:<br>
>> >>>><br>
>> >>>> Hmm, there's an old catch here. Someone will have to hollar if it is<br>
>> >>>> no<br>
>> >>>> longer relevant - but if it isn't, some documentation patch is called<br>
>> >>>> for.<br>
>> >>>><br>
>> >>>> In<br>
>> >>>> <a href="http://www.erlang.org/doc/apps/kernel/application.html#Module:start-2" target="_blank">http://www.erlang.org/doc/apps/kernel/application.html#Module:start-2</a><br>
>> >>>><br>
>> >>>> StartType defines the type of start:<br>
>> >>>><br>
>> >>>> normal if it's a normal startup.<br>
>> >>>> normal also if the application is distributed and started at the<br>
>> >>>> current<br>
>> >>>> node due to a failover from another node, and the application<br>
>> >>>> specification<br>
>> >>>> key start_phases == undefined.<br>
>> >>>> {takeover,Node} if the application is distributed and started at the<br>
>> >>>> current<br>
>> >>>> node due to a takeover from Node, either because<br>
>> >>>> application:takeover/2 has<br>
>> >>>> been called or because the current node has higher priority than<br>
>> >>>> Node.<br>
>> >>>> {failover,Node} if the application is distributed and started at the<br>
>> >>>> current<br>
>> >>>> node due to a failover from Node, and the application specification<br>
>> >>>> key start_phases /= undefined.<br>
>> >>>><br>
>> >>>> Note that StartType = {failover, Node} is only used if start_phases<br>
>> >>>> /=<br>
>> >>>> undefined. I will guess that it is for this reason that start_phases<br>
>> >>>> is<br>
>> >>>> actually set to undefined as default. If it isn't, it was a happy<br>
>> >>>> coincidence, since it actually preserves backward compatibility.<br>
>> >>>><br>
>> >>>> This particular oddity was introduced many years ago, in the 90s -<br>
>> >>>> possibly<br>
>> >>>> even before Erlang was released as Open Source, so the legacy<br>
>> >>>> argument may<br>
>> >>>> not be that relevant.<br>
>> >>>><br>
>> >>>> OTOH, I don't think many people use the start_phases attribute, so<br>
>> >>>> even new<br>
>> >>>> code might break if the Mod:start/2 function is suddenly called with<br>
>> >>>> a<br>
>> >>>> StartType that was never seen before.<br>
>> >>>><br>
>> >>>> BR,<br>
>> >>>> Ulf W<br>
>> >>>><br>
>> >>>><br>
>> >>>> On 3 Jan 2012, at 05:21, Juan Jose Comellas wrote:<br>
>> >>>><br>
>> >>>> The default value for 'start_phases' in .app files should be [], but<br>
>> >>>> it is incorrectly set to 'undefined' when generating a release. This<br>
>> >>>> happens when the original .app file does not set a value for<br>
>> >>>> 'start_phases' explicitly.<br>
>> >>>><br>
>> >>>> The reltool application defines its own copy of a record to handle<br>
>> >>>> .app files (#app_info{}, defined in lib/reltool/src/reltool.hrl) that<br>
>> >>>> has different default values than the one used by systools<br>
>> >>>> (#application{}, defined in lib/sasl/src/systools.hrl). In<br>
>> >>>> particular,<br>
>> >>>> the 'start_phases' entry is assumed by all of OTP to be [] when it is<br>
>> >>>> not explicitly set but reltool sets it to 'undefined' by default.<br>
>> >>>> This<br>
>> >>>> causes badmatch errors when trying to generate release upgrades.<br>
>> >>>><br>
>> >>>> Without this patch, all of the rebar examples that generate release<br>
>> >>>> upgrades that I've found on the web will fail. The bug was introduced<br>
>> >>>> in R14A.<br>
>> >>>><br>
>> >>>> git fetch git://<a href="http://github.com/jcomellas/otp.git" target="_blank">github.com/jcomellas/otp.git</a><br>
>> >>>> jc-fix-default-start-phases<br>
>> >>>><br>
>> >>>> Juanjo<br>
>> >>>> _______________________________________________<br>
>> >>>> erlang-patches mailing list<br>
>> >>>> <a href="mailto:erlang-patches@erlang.org">erlang-patches@erlang.org</a><br>
>> >>>> <a href="http://erlang.org/mailman/listinfo/erlang-patches" target="_blank">http://erlang.org/mailman/listinfo/erlang-patches</a><br>
>> >>>><br>
>> >>>><br>
>> >><br>
>><br>
>> _______________________________________________<br>
>> erlang-patches mailing list<br>
>> <a href="mailto:erlang-patches@erlang.org">erlang-patches@erlang.org</a><br>
>> <a href="http://erlang.org/mailman/listinfo/erlang-patches" target="_blank">http://erlang.org/mailman/listinfo/erlang-patches</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div></div></div>