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

From: Asif Naeem <anaeem(dot)it(at)gmail(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Fix broken Install.bat when target directory contains a space
Date: 2015-04-21 07:33:34
Message-ID: CAEB4t-Pb66bT5L1wA=pE7biwDLVGe3iF3PXddag4_arChWjoQg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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, BTW, it is a minor thing that can be handled in another patch, If
you like please feel free to change status to ready for committer. Thanks.

On Fri, Apr 17, 2015 at 10:36 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:

> 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
>

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Heikki Linnakangas 2015-04-21 07:38:25 Re: Streaming replication and WAL archive interactions
Previous Message Andres Freund 2015-04-21 07:19:00 Re: PATCH: Add 'pid' column to pg_replication_slots