On 3/31/2010 2:30 AM, Eli Zaretskii wrote:
>> +echo.   --distfiles             path to files to be included when running make dist, e.g. libXpm.dll
>>      
> Please make this text shorter, so it fits on a single 80-column line.
>    
Done.

>> +set distfilename=%~nx1
>>      
> I actually don't quite understand why you need to use %~nx1: it looks
> like distfilename is only used to check for messages to the user, so
> you could easily use distfilepath itself.  Is it just for beauty?
>    
Yes, just to show the filename. I removed all of that and just display a 
generic message.

>>   .PHONY: $(ALL)
>>
>> -
>>   addpm:		  stamp_BLD $(BLD)/addpm.exe
>>      
> Please don't make gratuitous white-space changes, at least not as part
> of a patch with real changes.
>    
Sorry, about that. That was not intentional.

>> +	- zipdist.bat $(INSTALL_DIR) $(VERSION)
>>      
> Why do you ignore errors in this command?  It is the single most
> important command in the `dist' target, so perhaps it should really
> error out.
>    
Yes it should. Fixed.

>> +set ARG_PATH="%~f1"
>>      
> Suggest SETLOCAL before the first command that sets environment
> variables.
Good point. Added.

>> +set ARG_PATH=%ARG_PATH:\=;%
>>      
> This warrants a comment, I think.
>    
I had some trouble parsing the path with '\' in it. Someone suggested 
replacing it and it worked miraculously. Still would like to know why 
the '\' didnt work, but my batch-fu is not that great. Do you have any idea?

>> +rem Check, if 7zip is installed and available on path
>> +:ZIP_CHECK
>> +7z a zipcheck zipdist.bat
>> +if %ERRORLEVEL% NEQ 0 goto :ZIP_ERROR
>> +rm zipcheck.7z
>>      
> I suggest instead to run 7z without arguments, which should fail only
> if 7z cannot be invoked by the user.  As a nice side-effect, you also
> get rid of the need to have rm.exe on PATH.
>    
Agreed. This is a better solution.

>> +rem Build distributions
>> +:ZIP_DIST
>> +set CUR_DIR=%CD%
>> +cd ..\..
>> [...]
>> +:EXIT
>> +cd %CUR_DIR%
>>      
> Why not use pushd/popd instead?
>    
Ignorance, I guess. ;) Fixed.

>> +rem Build&  verify full distribution
>>      
> Beware of shell-special characters in REM comments: no one said that
> the shell ignores them.  Older Windows shells didn't.  Just use "and"
> here, to be safe.
>    
Good point. Didn't think of that. Fixed.


v2 of the patch with all the fixes above is attached.

Thanks,
Christoph