<div>Sorry for the delay, but finally I have had a look at this....</div><div><br></div><div>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.</div>
<meta http-equiv="content-type" content="text/html; charset=utf-8"><div><br></div><div>I agree of course that systools.hrl is a bit misleading by stating a default value which is never used...</div><div><br></div><div>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!!!</div>
<div><br></div><div>Could this be what happens in your case, Juan?</div><div><br></div><div>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?</div>
<div><br></div><div>Regards</div><div>/siri</div><div><br></div><div><br></div><br><div class="gmail_quote">2012/1/5 Ulf Wiger <span dir="ltr"><<a href="mailto:ulf@feuerlabs.com">ulf@feuerlabs.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I would suggest that the special handling of {start_phases, undefined} be 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>
<div class="HOEnZb"><div class="h5"><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>
> <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 'start_phases' have been forgotten by those maintaining the code.<br>
>><br>
>> A closer analysis might reveal whether backward compatibility has already been sufficiently compromised that it is now time to Make It Right (™).<br>
>><br>
>> 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>
>><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 no<br>
>>>> longer relevant - but if it isn't, some documentation patch is called for.<br>
>>>><br>
>>>> In <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 current<br>
>>>> node due to a failover from another node, and the application specification<br>
>>>> key start_phases == undefined.<br>
>>>> {takeover,Node} if the application is distributed and started at the current<br>
>>>> node due to a takeover from Node, either because application:takeover/2 has<br>
>>>> been called or because the current node has higher priority than Node.<br>
>>>> {failover,Node} if the application is distributed and started at the 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>
>>>> undefined. I will guess that it is for this reason that start_phases 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 - possibly<br>
>>>> even before Erlang was released as Open Source, so the legacy argument may<br>
>>>> not be that relevant.<br>
>>>><br>
>>>> OTOH, I don't think many people use the start_phases attribute, so even new<br>
>>>> code might break if the Mod:start/2 function is suddenly called with 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 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. 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> 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>
</div></div></blockquote></div><br>