From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Mon, 14 Feb 2011 12:49:23 GMT
Subject: Re: [PATCH] fixes for ActiveState Perl
Message-ID: <AANLkTimCXEbzZnR1k32u942hjB37nThN_RDBL5N7owHa@mail.gmail.com>
In-Reply-To: <4D5914C1.9050008@io.com>

On Mon, Feb 14, 2011 at 12:40 PM, Rafael Kitover <rkitover@io.com> wrote:
> The make for perl is now retrieved from Perl's config. The make fragment in
> perl/Makefile.PL added in MY::postamble is disabled on Win32, as it relies on
> GNU make syntax, and Win32 users are not likely to have an ancient EU::MM
> version.
>
> A Windows path for PERL_PATH is now supported as well, e.g.:
> make PERL_PATH=C:\\Perl\\bin\\perl
> or from cmd.exe:
> make PERL_PATH=C:\Perl\bin\perl
> .
>
> t9700-perl-git.sh now passes on ActiveState Perl. Some tweaks were necessary in
> Git.pm: a more correct check for absolute directory, exit code check on closing
> the "pipe", and always closing the cat_blob bidirectional pipe (with errors
>  ignored) so as not to leave zombie processes. The waitpid call on closing the
> bidirectional pipe is now timed and the process is killed if necessary. Also
> added some binmode calls to t/t9700/test.pl to make the blob tests pass.
>
> Signed-off-by: Rafael Kitover <rkitover@cpan.org>
> ---
>  Makefile         |    8 ++++----
>  perl/Git.pm      |   40 +++++++++++++++++++++++++++++++---------
>  perl/Makefile    |   14 ++++++++++++--
>  perl/Makefile.PL |    7 ++++++-
>  t/t9700/test.pl  |   15 ++++++++++++++-
>  5 files changed, 67 insertions(+), 17 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index d3dcfb1..3465ab5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -175,7 +175,7 @@ all::
>  # will work.
>  #
>  # Define NO_PERL_MAKEMAKER if you cannot use Makefiles generated by perl's
> -# MakeMaker (e.g. using ActiveState under Cygwin).
> +# MakeMaker.
>  #
>  # Define NO_PERL if you do not want Perl scripts or libraries at all.
>  #
> @@ -1059,7 +1059,7 @@ ifeq ($(uname_S),Windows)
>        NO_MKSTEMPS = YesPlease
>        SNPRINTF_RETURNS_BOGUS = YesPlease
>        NO_SVN_TESTS = YesPlease
> -       NO_PERL_MAKEMAKER = YesPlease
> +       # NO_PERL_MAKEMAKER = YesPlease
>        RUNTIME_PREFIX = YesPlease
>        NO_POSIX_ONLY_PROGRAMS = YesPlease
>        NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease

This would break my MSVC builds, since I'm using msysGit + MSVC to
build (when using MSVC)... Not that that's a big deal, I can add
"NO_PERL_MAKEMAKER = YesPlease" to my config.mak.

But I do suspect that most people who build with MSVC will have the
same problem. Is it worth the hassle?

> @@ -1112,7 +1112,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
>        NO_MKDTEMP = YesPlease
>        NO_MKSTEMPS = YesPlease
>        NO_SVN_TESTS = YesPlease
> -       NO_PERL_MAKEMAKER = YesPlease
> +       # NO_PERL_MAKEMAKER = YesPlease
>        RUNTIME_PREFIX = YesPlease
>        NO_POSIX_ONLY_PROGRAMS = YesPlease
>        NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease

This would break Git for Windows, as we don't have makemaker
ExtUtils::MakeMaker in msysGit.

You could probably move the definition into the THIS_IS_MSYSGIT-check, though.

> @@ -1652,7 +1652,7 @@ perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
>
>  $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
>        $(QUIET_GEN)$(RM) $@ $@+ && \
> -       INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
> +       INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir | sed -e 's/\\\\/\\\\\\\\/g' -e "s/'//g"` && \
>        sed -e '1{' \
>            -e '        s|#!.*perl|#!$(PERL_PATH_SQ)|' \
>            -e '        h' \
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 6cb0dd1..f7d99bd 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -101,6 +101,7 @@ use Error qw(:try);
>  use Cwd qw(abs_path);
>  use IPC::Open2 qw(open2);
>  use Fcntl qw(SEEK_SET SEEK_CUR);
> +use File::Spec ();
>  }
>
>
> @@ -184,7 +185,8 @@ sub repository {
>                };
>
>                if ($dir) {
> -                       $dir =~ m#^/# or $dir = $opts{Directory} . '/' . $dir;
> +                        File::Spec->file_name_is_absolute($dir)
> +                               or $dir = $opts{Directory} . '/' . $dir;
>                        $opts{Repository} = abs_path($dir);
>
>                        # If --git-dir went ok, this shouldn't die either.

Just for reference, this should still work as before on Git for
Windows, because we have a MSYS perl, so absolute paths starts with a
forward slash. And if we change to a MinGW perl, this would probably
be an improvement.

> diff --git a/perl/Makefile b/perl/Makefile
> index a2ffb64..1fa99cd 100644
> --- a/perl/Makefile
> +++ b/perl/Makefile
> @@ -3,18 +3,28 @@
>  #
>  makfile:=perl.mak
>
> +# support PERL_PATH=C:\\Perl\\bin\\perl
> +PERL_PATH := $(subst \,\\,$(PERL_PATH))
> +
>  PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
> +PERL_MAKE := MAKEFLAGS="" $(subst \,\\,$(shell $(subst \,\\,$(PERL_PATH)) -MConfig -le "print ((\%Config)->{make})"))
> +
> +ifneq (,$(findstring nmake,$(PERL_MAKE)))
> +       PERL_MAKE := $(PERL_MAKE) -nologo
> +endif
> +
>  prefix_SQ = $(subst ','\'',$(prefix))
>
>  ifndef V
>        QUIET = @
>  endif
>
> +

Why?