Fix broken Install.bat when target directory contains a space

Started by Michael Paquieralmost 11 years ago13 messages

michael.paquier@gmail.com

almost 11 years ago

1 attachment(s)

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

Attachments:

20150302_fix_msvc_install.patchtext/x-diff; charset=US-ASCII; name=20150302_fix_msvc_install.patchDownload

diff --git a/src/tools/msvc/install.bat b/src/tools/msvc/install.bat
index bed08f1..29c6c31 100644
--- a/src/tools/msvc/install.bat
+++ b/src/tools/msvc/install.bat
@@ -1,7 +1,7 @@
 @echo off
 REM src/tools/msvc/install.bat
 
-if NOT "%1"=="" GOTO RUN_INSTALL
+if NOT [%1]==[/?] GOTO RUN_INSTALL
 
 echo Invalid command line options.
 echo Usage: "install.bat <path>"
@@ -20,7 +20,7 @@ CALL bldenv.bat
 del bldenv.bat
 :nobuildenv
 
-perl install.pl "%1" %2
+perl install.pl %1 %2
 
 REM exit fix for pre-2003 shell especially if used on buildfarm
 if "%XP_EXIT_FIX%" == "yes" exit %ERRORLEVEL%

Asif Naeem

anaeem.it@gmail.com

over 10 years ago

In reply to: Michael Paquier (#1)

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

Hi Michael,

I spend spend time look into the patch. Good catch, I am also surprised to
see that current Windows install script don’t support spaces in the path.
Please see my findings as following i.e.

*Without the patch*

C:\PG\postgresql\src\tools\msvc>install "C:\PG\postgresql\inst with space
without patch"
with was unexpected at this time.

C:\PG\postgresql\src\tools\msvc>install
Invalid command line options.
Usage: "install.bat <path>"

C:\PG\postgresql\src\tools\msvc>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 93
Install::Install('/?', undef) called at install.pl line 13

*With the patch*

C:\PG\postgresql\src\tools\msvc>install "C:\PG\postgresql\inst with space
without patch"
Works fine.

C:\PG\postgresql\src\tools\msvc>install
Usage: install.pl <targetdir> [installtype]
installtype: client

C:\PG\postgresql\src\tools\msvc>install /?
Invalid command line options.
Usage: "install.bat <path>"

Following change looks confusing to me i.e.

-if NOT "%1"=="" GOTO RUN_INSTALL
+if NOT [%1]==[/?] GOTO RUN_INSTALL

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
<http://install.pl>* <targetdir> [installtype]' but (3) shows usage message
as 'Usage: "*install.bat* <path>"'. Thanks.

Regards,
Muhammad Asif Naeem

On Mon, Mar 2, 2015 at 9:27 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

Show quoted text

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Asif Naeem (#2)

1 attachment(s)

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

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

Attachments:

20150417_fix_msvc_install_v2.patchtext/x-diff; charset=US-ASCII; name=20150417_fix_msvc_install_v2.patchDownload

diff --git a/src/tools/msvc/install.bat b/src/tools/msvc/install.bat
index bed08f1..cddb453 100644
--- a/src/tools/msvc/install.bat
+++ b/src/tools/msvc/install.bat
@@ -1,10 +1,11 @@
 @echo off
 REM src/tools/msvc/install.bat
 
-if NOT "%1"=="" GOTO RUN_INSTALL
+if NOT [%1]==[] GOTO RUN_INSTALL
 
 echo Invalid command line options.
-echo Usage: "install.bat <path>"
+echo Usage: "install.bat <targetdir> [installtype]"
+echo installtype: client
 echo.
 REM exit fix for pre-2003 shell especially if used on buildfarm
 if "%XP_EXIT_FIX%" == "yes" exit 1
@@ -20,7 +22,7 @@ CALL bldenv.bat
 del bldenv.bat
 :nobuildenv
 
-perl install.pl "%1" %2
+perl install.pl %1 %2
 
 REM exit fix for pre-2003 shell especially if used on buildfarm
 if "%XP_EXIT_FIX%" == "yes" exit %ERRORLEVEL%

Asif Naeem

anaeem.it@gmail.com

over 10 years ago

In reply to: Michael Paquier (#3)

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

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.paquier@gmail.com

Show quoted text

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

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Asif Naeem (#4)

1 attachment(s)

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

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.

If you like please feel free to change status to ready for committer.

Well, I don't think that the patch author should do that. So I won't do it
by myself.

Attached is an updated patch.
Regards,
--
Michael

Attachments:

20150421_fix_msvc_install_v3.patchtext/x-diff; charset=US-ASCII; name=20150421_fix_msvc_install_v3.patchDownload

diff --git a/src/tools/msvc/install.bat b/src/tools/msvc/install.bat
index bed08f1..b84fef0 100644
--- a/src/tools/msvc/install.bat
+++ b/src/tools/msvc/install.bat
@@ -1,10 +1,11 @@
 @echo off
 REM src/tools/msvc/install.bat
 
-if NOT "%1"=="" GOTO RUN_INSTALL
+if NOT [%1]==[] GOTO RUN_INSTALL
 
 echo Invalid command line options.
-echo Usage: "install.bat <path>"
+echo Usage: "install.bat <targetdir> [installtype]"
+echo installtype: all client
 echo.
 REM exit fix for pre-2003 shell especially if used on buildfarm
 if "%XP_EXIT_FIX%" == "yes" exit 1
@@ -20,7 +21,7 @@ CALL bldenv.bat
 del bldenv.bat
 :nobuildenv
 
-perl install.pl "%1" %2
+perl install.pl %1 %2
 
 REM exit fix for pre-2003 shell especially if used on buildfarm
 if "%XP_EXIT_FIX%" == "yes" exit %ERRORLEVEL%
diff --git a/src/tools/msvc/install.pl b/src/tools/msvc/install.pl
index 97e297e..62ef21e 100755
--- a/src/tools/msvc/install.pl
+++ b/src/tools/msvc/install.pl
@@ -15,6 +15,6 @@ Install($target, $insttype);
 sub Usage
 {
 	print "Usage: install.pl <targetdir> [installtype]\n";
-	print "installtype: client\n";
+	print "installtype: all client\n";
 	exit(1);
 }

Asif Naeem

anaeem.it@gmail.com

over 10 years ago

In reply to: Michael Paquier (#5)

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

Thank you Michael, latest patch looks good to me. I have changed its status
to ready for committer.

Regards,
Muhammad Asif Naeem

On Tue, Apr 21, 2015 at 6:02 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

Show quoted text

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.

If you like please feel free to change status to ready for committer.

Well, I don't think that the patch author should do that. So I won't do it
by myself.

Attached is an updated patch.
Regards,
--
Michael

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Asif Naeem (#6)

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

On Wed, Apr 22, 2015 at 12:40 AM, Asif Naeem <anaeem.it@gmail.com> wrote:

Thank you Michael, latest patch looks good to me. I have changed its
status to ready for committer.

Thanks!
--
Michael

Heikki Linnakangas

hlinnaka@iki.fi

over 10 years ago

In reply to: Michael Paquier (#5)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Heikki Linnakangas (#8)

1 attachment(s)

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

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

Attachments:

20150706_fix_msvc_install_v3.patchtext/x-diff; charset=US-ASCII; name=20150706_fix_msvc_install_v3.patchDownload

diff --git a/src/tools/msvc/install.bat b/src/tools/msvc/install.bat
index bed08f1..d03277e 100644
--- a/src/tools/msvc/install.bat
+++ b/src/tools/msvc/install.bat
@@ -1,27 +1,6 @@
 @echo off
 REM src/tools/msvc/install.bat
-
-if NOT "%1"=="" GOTO RUN_INSTALL
-
-echo Invalid command line options.
-echo Usage: "install.bat <path>"
-echo.
-REM exit fix for pre-2003 shell especially if used on buildfarm
-if "%XP_EXIT_FIX%" == "yes" exit 1
-exit /b 1
-
-:RUN_INSTALL
-
-SETLOCAL
-
-IF NOT EXIST buildenv.pl goto nobuildenv
-perl -e "require 'buildenv.pl'; while(($k,$v) = each %%ENV) { print qq[\@SET $k=$v\n]; }" > bldenv.bat
-CALL bldenv.bat
-del bldenv.bat
-:nobuildenv
-
-perl install.pl "%1" %2
-
-REM exit fix for pre-2003 shell especially if used on buildfarm
-if "%XP_EXIT_FIX%" == "yes" exit %ERRORLEVEL%
-exit /b %ERRORLEVEL%
+REM all the logic for this now belongs in install.pl. This file really
+REM only exists so you don't have to type "perl install.pl"
+REM Resist any temptation to add any logic here.
+@perl install.pl %*

#10

Heikki Linnakangas

hlinnaka@iki.fi

over 10 years ago

In reply to: Michael Paquier (#9)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Heikki Linnakangas (#10)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Michael Paquier (#11)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13

Heikki Linnakangas

hlinnaka@iki.fi

over 10 years ago

In reply to: Tom Lane (#12)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers