Thread: Fix broken Install.bat when target directory contains a space

Hi all,

When using install.bat with a path containing spaces, I got surprised
by a couple of errors.
1) First with this path:
$ install "c:\Program Files\pgsql"
I am getting the following error:
Files\pgsql""=="" was unexpected at this time.
This is caused by an incorrect evaluation of the first parameter in install.bat.
2) After correcting the first error, the path is truncated to
c:\Program because first argument value does not seem to be correctly
parsed when used with install.pl.

Attached is a patch fixing both problems. I imagine that it would be
good to get that backpatched.
Regards,
--
Michael

On Thu, Apr 16, 2015 at 5:40 PM, Asif Naeem wrote:
> Along with fixing the space in installation path, it is also changing the
> behavior of install script, why not just "if NOT [%1]==[] GOTO RUN_INSTALL",
> checking for "/?" seems good for help message but it seem not well handled
> in the script, it is still saying "Invalid command line options.", along
> with this, option "/?" seems not handled by any other .bat build script.
> Other than this, with the patch applied, is it an acceptable behavior that
> (2) shows usage message as 'Usage: install.pl <targetdir> [installtype]' but
> (3) shows usage message as 'Usage: "install.bat <path>"'. Thanks.

Thanks for your review!

OK, let's remove the use of /? then, consistency with the other
scripts is a good argument for its removal.Attached is an updated
patch that does the following regarding missing arguments:
>install
Invalid command line options.
Usage: "install.bat <targetdir> [installtype]"
installtype: client
>install
Installing version 9.5 for release in /?
Copying build output files...Could not copy release\postgres\postgres.exe to /?\
bin\postgres.exe
 at Install.pm line 40.
        Install::lcopy("release\\postgres\\postgres.exe", "/?\\bin\\postgres.exe
") called at Install.pm line 324
        Install::CopySolutionOutput("release", "/?") called at Install.pm line 9
3
        Install::Install("/?", undef) called at install.pl line 13

This patch fixes of course the issue with spaces included in the
target path. I updated as well the Usage in install.bat to be
consistent with install.pl.
Regards,
--
Michael

<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Wed, Apr 22, 2015 at 12:40 AM, Asif
Naeem<span dir="ltr"><<a href="mailto:anaeem.it@gmail.com" target="_blank">anaeem.it@gmail.com</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div
dir="ltr">Thankyou Michael, latest patch looks good to me. I have changed its status to ready for
committer. </div></blockquote></div><br/></div><div class="gmail_extra">Thanks!<br />-- <br /><div
class="gmail_signature">Michael<br/></div></div></div> 

On 04/21/2015 04:02 PM, Michael Paquier wrote:
> On Tue, Apr 21, 2015 at 4:33 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
>
>> The v2 patch looks good to me, just a minor concern on usage message i.e.
>>
>> C:\PG\postgresql\src\tools\msvc>install
>>> Invalid command line options.
>>> Usage: "install.bat <targetdir> [installtype]"
>>> installtype: client
>>
>>
>> It seems that there are two install options i.e. client, all (any other
>> string other than client is being considered or treated as all), the
>> following install command works i.e.
>>
>> install C:\PG\postgresql\inst option_does_not_exist
>>
>>
>> As your patch effects this area of code, I thought to share these findings
>> with you,o BTW, it is a minor thing that can be handled in another patch,
>
> Well, that's the same behavior that this script has been having for ages.
> Let's just update the usage message to mention both "all" and "client". I
> see no point in breaking a behavior that has been like that for ages, and
> the main point of this patch is to fix the install path issue.

Hmm. Why is install.bat not like build.bat, i.e. just a thin wrapper 
that just calls install.pl, passing all arguments?

- Heikki

On Sat, Jul 4, 2015 at 3:57 AM, Heikki Linnakangas wrote:
> Hmm. Why is install.bat not like build.bat, i.e. just a thin wrapper that
> just calls install.pl, passing all arguments?

I guess we just haven't noticed it. And indeed it makes everything
more simple, and fixes as well the error reported when install path
contains a space.
--
Michael

On 07/06/2015 04:38 AM, Michael Paquier wrote:
> On Sat, Jul 4, 2015 at 3:57 AM, Heikki Linnakangas wrote:
>> Hmm. Why is install.bat not like build.bat, i.e. just a thin wrapper that
>> just calls install.pl, passing all arguments?
>
> I guess we just haven't noticed it. And indeed it makes everything
> more simple, and fixes as well the error reported when install path
> contains a space.

Ok, committed that way.

- Heikki

On Tue, Jul 7, 2015 at 4:19 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 07/06/2015 04:38 AM, Michael Paquier wrote:
>>
>> On Sat, Jul 4, 2015 at 3:57 AM, Heikki Linnakangas wrote:
>>>
>>> Hmm. Why is install.bat not like build.bat, i.e. just a thin wrapper that
>>> just calls install.pl, passing all arguments?
>>
>>
>> I guess we just haven't noticed it. And indeed it makes everything
>> more simple, and fixes as well the error reported when install path
>> contains a space.
>
>
> Ok, committed that way.

Shoudn't this patch be backpatched? In the backbranches install.bat
does not work correctly with paths containing spaces.
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Tue, Jul 7, 2015 at 4:19 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Ok, committed that way.

> Shoudn't this patch be backpatched? In the backbranches install.bat
> does not work correctly with paths containing spaces.

I was about to ask the same.  AFAICS, the other scripts have been similar
one-liners at least since 9.0.
        regards, tom lane

On 07/07/2015 02:56 AM, Tom Lane wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Tue, Jul 7, 2015 at 4:19 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>> Ok, committed that way.
>
>> Shoudn't this patch be backpatched? In the backbranches install.bat
>> does not work correctly with paths containing spaces.
>
> I was about to ask the same.  AFAICS, the other scripts have been similar
> one-liners at least since 9.0.

Ok then, pushed to back-branches too.

- Heikki