[RFC PATCH 0/7] test: NetBSD support

[RFC PATCH 0/7] test: NetBSD support

[Carlo Marcelo Arenas Belón]

Likely still missing changes as it only completes a run with a minimal
number of dependencies but open for feedback

Requires pkgsrc packages for gmake, perl, bash and curl and completes a run

  $ gmake SHELL_PATH=/usr/pkg/bin/bash NO_PYTHON=1 CURL_DIR=/usr/pkg test

Carlo Marcelo Arenas Belón (7):
  Documentation: update INSTALL for NetBSD
  t0301: remove trailing / for dir creation
  config.mak.uname: NetBSD uses BSD semantics with fread for directories
  config.mak.uname: NetBSD uses old iconv interface
  test-lib: use pkgsrc provided unzip for NetBSD
  t5004: use GNU tar to avoid known issues with BSD tar
  config.mak.uname: use pkgsrc perl for NetBSD

 INSTALL                         | 6 +++---
 config.mak.uname                | 3 +++
 t/t0301-credential-cache.sh     | 8 ++++----
 t/t5004-archive-corner-cases.sh | 2 ++
 t/test-lib.sh                   | 1 +
 5 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.20.0.rc1

[RFC PATCH 1/7] Documentation: update INSTALL for NetBSD

[Carlo Marcelo Arenas Belón]

NetBSD added a BSD licensed reimplementation of GNU libintl to
its base at least since release 4.0 (mid 2012) and git can be
configured to build with it.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 INSTALL | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/INSTALL b/INSTALL
index c39006e8e7..100718989f 100644
--- a/INSTALL
+++ b/INSTALL
@@ -154,11 +154,11 @@ Issues of note:
 	  git-gui, you can use NO_TCLTK.
 
 	- A gettext library is used by default for localizing Git. The
-	  primary target is GNU libintl, but the Solaris gettext
-	  implementation also works.
+	  primary target is GNU libintl, but the Solaris and NetBSD gettext
+	  implementations also work.
 
 	  We need a gettext.h on the system for C code, gettext.sh (or
-	  Solaris gettext(1)) for shell scripts, and libintl-perl for Perl
+	  gettext(1) utility) for shell scripts, and libintl-perl for Perl
 	  programs.
 
 	  Set NO_GETTEXT to disable localization support and make Git only
-- 
2.20.0.rc1

Re: [RFC PATCH 1/7] Documentation: update INSTALL for NetBSD

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> NetBSD added a BSD licensed reimplementation of GNU libintl to
> its base at least since release 4.0 (mid 2012) and git can be
> configured to build with it.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  INSTALL | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Looks good.

> diff --git a/INSTALL b/INSTALL
> index c39006e8e7..100718989f 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -154,11 +154,11 @@ Issues of note:
>  	  git-gui, you can use NO_TCLTK.
>  
>  	- A gettext library is used by default for localizing Git. The
> -	  primary target is GNU libintl, but the Solaris gettext
> -	  implementation also works.
> +	  primary target is GNU libintl, but the Solaris and NetBSD gettext
> +	  implementations also work.
>  
>  	  We need a gettext.h on the system for C code, gettext.sh (or
> -	  Solaris gettext(1)) for shell scripts, and libintl-perl for Perl
> +	  gettext(1) utility) for shell scripts, and libintl-perl for Perl
>  	  programs.
>  
>  	  Set NO_GETTEXT to disable localization support and make Git only

[RFC PATCH 2/7] t0301: remove trailing / for dir creation

[Carlo Marcelo Arenas Belón]

the semantics of how mkdir -p should work, specially when using -m are
not standard and in this case NetBSD will assume that the permision
should not be changed, breaking the test

-p is technically not needed either, but will be cleared in a future
patch eventhough it could be considered an alternative fix

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t0301-credential-cache.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index fd92533acf..9529c612af 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -77,9 +77,9 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
 test_expect_success 'use user socket if user directory exists' '
 	test_when_finished "
 		git credential-cache exit &&
-		rmdir \"\$HOME/.git-credential-cache/\"
+		rmdir \"\$HOME/.git-credential-cache\"
 	" &&
-	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
+	mkdir -p -m 700 "$HOME/.git-credential-cache" &&
 	check approve cache <<-\EOF &&
 	protocol=https
 	host=example.com
@@ -92,10 +92,10 @@ test_expect_success 'use user socket if user directory exists' '
 test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' '
 	test_when_finished "
 		git credential-cache exit &&
-		rmdir \"\$HOME/dir/\" &&
+		rmdir \"\$HOME/dir\" &&
 		rm \"\$HOME/.git-credential-cache\"
 	" &&
-	mkdir -p -m 700 "$HOME/dir/" &&
+	mkdir -p -m 700 "$HOME/dir" &&
 	ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
 	check approve cache <<-\EOF &&
 	protocol=https
-- 
2.20.0.rc1

Re: [RFC PATCH 2/7] t0301: remove trailing / for dir creation

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> the semantics of how mkdir -p should work, specially when using -m are
> not standard and in this case NetBSD will assume that the permision
> should not be changed, breaking the test

This does not explain, except for the fuzzy "in this case", why we
want to lose trailing slash at all.  If what you wanted to say was

    On NetBSD, "mkdir -p -m $bits path/to/dir/" ignores the
    permission bits when creating the directory component 'dir', but
    without the trailing slash at the end of "dir/", it works as
    expected.

then that would be an understandable justification for the patch.
If you meant to say something else, then I couldn't read it from
what you wrote, so the log message needs updating to help future
readers.

> -p is technically not needed either, but will be cleared in a future
> patch eventhough it could be considered an alternative fix

I haven't seen the steps 3-7/7 yet, but if they remove "-p", then
"but" would be a strange thing to say here.  If they indeed do, then
perhaps:

    The '-p' option is not needed in thse cases, as we know $HOME/
    exists in the test environment and we are creating a new
    directory directly under it.  It will be removed in a future
    patch in the series.  Removing '-p' could be an alternative fix,
    as the command then works as expected even on NetBSD with the
    trailing slash after directory name.

On the other hand, if future changes make it necessary to create two
or more levels here, then

    At this point in the series, '-p' is not needed in thse cases,
    as we know $HOME/ exists in the test environment and we are
    creating .git-credential-cache directory directly under it.
    However, in later patches, we'll make it necessary for these
    'mkdir -p' to create two or more levels, so removing '-p' would
    be an alternative fix for this step, but would not work for the
    series as a whole.

would also make sense.  I simply do not have enough information yet
to tell which at this point in the series.

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/t0301-credential-cache.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
> index fd92533acf..9529c612af 100755
> --- a/t/t0301-credential-cache.sh
> +++ b/t/t0301-credential-cache.sh
> @@ -77,9 +77,9 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
>  test_expect_success 'use user socket if user directory exists' '
>  	test_when_finished "
>  		git credential-cache exit &&
> -		rmdir \"\$HOME/.git-credential-cache/\"
> +		rmdir \"\$HOME/.git-credential-cache\"
>  	" &&
> -	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
> +	mkdir -p -m 700 "$HOME/.git-credential-cache" &&
>  	check approve cache <<-\EOF &&
>  	protocol=https
>  	host=example.com
> @@ -92,10 +92,10 @@ test_expect_success 'use user socket if user directory exists' '
>  test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' '
>  	test_when_finished "
>  		git credential-cache exit &&
> -		rmdir \"\$HOME/dir/\" &&
> +		rmdir \"\$HOME/dir\" &&
>  		rm \"\$HOME/.git-credential-cache\"
>  	" &&
> -	mkdir -p -m 700 "$HOME/dir/" &&
> +	mkdir -p -m 700 "$HOME/dir" &&
>  	ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
>  	check approve cache <<-\EOF &&
>  	protocol=https

[RFC PATCH 3/7] config.mak.uname: NetBSD uses BSD semantics with fread for directories

[Carlo Marcelo Arenas Belón]

this "fixes" test 23 (proper error on directory "files") from t1308

other BSD (OpenBSD, MirBSD) likely also affected but they will be
fixed in a different series

the optional 'configure' sets this automatically and is probably what
most users from this platform had been doing as a workaround

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index 3ee7da0e23..36c973c7e6 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -253,6 +253,7 @@ ifeq ($(uname_S),NetBSD)
 	HAVE_BSD_SYSCTL = YesPlease
 	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
 	PROCFS_EXECUTABLE_PATH = /proc/curproc/exe
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),AIX)
 	DEFAULT_PAGER = more
-- 
2.20.0.rc1

Re: [RFC PATCH 3/7] config.mak.uname: NetBSD uses BSD semantics with fread for directories

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> this "fixes" test 23 (proper error on directory "files") from t1308
>
> other BSD (OpenBSD, MirBSD) likely also affected but they will be
> fixed in a different series
>
> the optional 'configure' sets this automatically and is probably what
> most users from this platform had been doing as a workaround

Yup, thanks for straightening this out.

>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 3ee7da0e23..36c973c7e6 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -253,6 +253,7 @@ ifeq ($(uname_S),NetBSD)
>  	HAVE_BSD_SYSCTL = YesPlease
>  	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
>  	PROCFS_EXECUTABLE_PATH = /proc/curproc/exe
> +	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>  endif
>  ifeq ($(uname_S),AIX)
>  	DEFAULT_PAGER = more

[RFC PATCH 4/7] config.mak.uname: NetBSD uses old iconv interface

[Carlo Marcelo Arenas Belón]

prevents the following warning :

utf8.c: In function 'reencode_string_iconv':
utf8.c:486:28: warning: passing argument 2 of 'iconv' from incompatible pointer type [-Wincompatible-pointer-types]
   size_t cnt = iconv(conv, &cp, &insz, &outpos, &outsz);
                            ^
In file included from git-compat-util.h:275:0,
                 from utf8.c:1:
/usr/include/iconv.h:46:8: note: expected 'const char ** restrict' but argument is of type 'char **'
 size_t iconv(iconv_t, const char ** __restrict,
        ^

it is set by optional configure at least since NetBSD 6.0

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index 36c973c7e6..59ce03819b 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -246,6 +246,7 @@ ifeq ($(uname_S),NetBSD)
 	ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2)
 		NEEDS_LIBICONV = YesPlease
 	endif
+	OLD_ICONV = UnfortunatelyYes
 	BASIC_CFLAGS += -I/usr/pkg/include
 	BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
 	USE_ST_TIMESPEC = YesPlease
-- 
2.20.0.rc1

Re: [RFC PATCH 4/7] config.mak.uname: NetBSD uses old iconv interface

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> prevents the following warning :

s/^/Doing so / or something to make it a complete sentence.

> ...
> it is set by optional configure at least since NetBSD 6.0

s/it/It/;


Again, makes sense, and thanks for tying this loose end.

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 36c973c7e6..59ce03819b 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -246,6 +246,7 @@ ifeq ($(uname_S),NetBSD)
>  	ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2)
>  		NEEDS_LIBICONV = YesPlease
>  	endif
> +	OLD_ICONV = UnfortunatelyYes
>  	BASIC_CFLAGS += -I/usr/pkg/include
>  	BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
>  	USE_ST_TIMESPEC = YesPlease

[RFC PATCH 5/7] test-lib: use pkgsrc provided unzip for NetBSD

[Carlo Marcelo Arenas Belón]

d98b2c5fce ("test-lib: on FreeBSD, look for unzip(1) in /usr/local/bin/",
2016-07-21) added an exception to the test suite for FreeBSD because the
tests assume functionality not provided by its base unzip tool.

NetBSD shares that limitation and provides a package that could be used
instead so all tests from t5003 succeed

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 6c6c0af7a1..2acb35f277 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1244,6 +1244,7 @@ test_lazy_prereq SANITY '
 '
 
 test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}
+test $uname_s = NetBSD && GIT_UNZIP=${GIT_UNZIP:-/usr/pkg/bin/unzip}
 GIT_UNZIP=${GIT_UNZIP:-unzip}
 test_lazy_prereq UNZIP '
 	"$GIT_UNZIP" -v
-- 
2.20.0.rc1

Re: [RFC PATCH 5/7] test-lib: use pkgsrc provided unzip for NetBSD

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> d98b2c5fce ("test-lib: on FreeBSD, look for unzip(1) in /usr/local/bin/",
> 2016-07-21) added an exception to the test suite for FreeBSD because the
> tests assume functionality not provided by its base unzip tool.
>
> NetBSD shares that limitation and provides a package that could be used
> instead so all tests from t5003 succeed
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/test-lib.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 6c6c0af7a1..2acb35f277 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1244,6 +1244,7 @@ test_lazy_prereq SANITY '
>  '
>  
>  test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}
> +test $uname_s = NetBSD && GIT_UNZIP=${GIT_UNZIP:-/usr/pkg/bin/unzip}

This is OK for now, but I wonder why this is done in test-lib.sh in
the first place, unlike $(TAR) that is set and configurable from the
top level.  The difference is that GIT_UNZIP happens to be used only
in tests, while TAR is used in the primary build procedure.

But I suspect that in the longer run, we should allow UNZIP to be
given next to TAR to the top-level Makefile and export it down.
That would allow the usual mechanism like config.mak.uname,
./configure and "make TAR=... UNZIP=..." command line override
to be used uniformly, without people having to worry about the
distinction.  The builders should *not* have to care that one is
used in the build and the other is only used in the test.

>  GIT_UNZIP=${GIT_UNZIP:-unzip}
>  test_lazy_prereq UNZIP '
>  	"$GIT_UNZIP" -v

[RFC PATCH 6/7] t5004: use GNU tar to avoid known issues with BSD tar

[Carlo Marcelo Arenas Belón]

56ee96572a ("t5004: resurrect original empty tar archive test", 2013-05-09)
added a test to try to detect and workaround issues with the standard tar
from BSD, but at least in NetBSD would be better to instead require GNU tar
which is available from pkgsrc

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t5004-archive-corner-cases.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
index ced44355ca..baafc553f8 100755
--- a/t/t5004-archive-corner-cases.sh
+++ b/t/t5004-archive-corner-cases.sh
@@ -31,6 +31,8 @@ test_lazy_prereq UNZIP_ZIP64_SUPPORT '
 	"$GIT_UNZIP" -v | grep ZIP64_SUPPORT
 '
 
+test $uname_s = NetBSD && TAR="gtar"
+
 # bsdtar/libarchive versions before 3.1.3 consider a tar file with a
 # global pax header that is not followed by a file record as corrupt.
 if "$TAR" tf "$TEST_DIRECTORY"/t5004/empty-with-pax-header.tar >/dev/null 2>&1
-- 
2.20.0.rc1

Re: [RFC PATCH 6/7] t5004: use GNU tar to avoid known issues with BSD tar

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 56ee96572a ("t5004: resurrect original empty tar archive test", 2013-05-09)
> added a test to try to detect and workaround issues with the standard tar
> from BSD, but at least in NetBSD would be better to instead require GNU tar
> which is available from pkgsrc
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/t5004-archive-corner-cases.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index ced44355ca..baafc553f8 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -31,6 +31,8 @@ test_lazy_prereq UNZIP_ZIP64_SUPPORT '
>  	"$GIT_UNZIP" -v | grep ZIP64_SUPPORT
>  '
>  
> +test $uname_s = NetBSD && TAR="gtar"
> +

This smells wrong.

Isn't the top-level Makefile ask you to use TAR=gtar if that is the
usable implementation of tar on your platform, and isn't what you
specify as $(TAR) exported down to the t/Makefile to be used here?

>  # bsdtar/libarchive versions before 3.1.3 consider a tar file with a
>  # global pax header that is not followed by a file record as corrupt.
>  if "$TAR" tf "$TEST_DIRECTORY"/t5004/empty-with-pax-header.tar >/dev/null 2>&1

[RFC PATCH 7/7] config.mak.uname: use pkgsrc perl for NetBSD

[Carlo Marcelo Arenas Belón]

otherwise will default to /usr/bin/perl which wouldn't normally exist

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index 59ce03819b..d2edb723f4 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -249,6 +249,7 @@ ifeq ($(uname_S),NetBSD)
 	OLD_ICONV = UnfortunatelyYes
 	BASIC_CFLAGS += -I/usr/pkg/include
 	BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
+	PERL_PATH = /usr/pkg/bin/perl
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_PATHS_H = YesPlease
 	HAVE_BSD_SYSCTL = YesPlease
-- 
2.20.0.rc1

Re: [RFC PATCH 7/7] config.mak.uname: use pkgsrc perl for NetBSD

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> otherwise will default to /usr/bin/perl which wouldn't normally exist
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)

I do not have experience with NetBSD so I take your words on face
value.  This patch makes sense to me.

Thanks.

> diff --git a/config.mak.uname b/config.mak.uname
> index 59ce03819b..d2edb723f4 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -249,6 +249,7 @@ ifeq ($(uname_S),NetBSD)
>  	OLD_ICONV = UnfortunatelyYes
>  	BASIC_CFLAGS += -I/usr/pkg/include
>  	BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
> +	PERL_PATH = /usr/pkg/bin/perl
>  	USE_ST_TIMESPEC = YesPlease
>  	HAVE_PATHS_H = YesPlease
>  	HAVE_BSD_SYSCTL = YesPlease

Re: [RFC PATCH 0/7] test: NetBSD support

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 25 2018, Carlo Marcelo Arenas Belón wrote:

> Likely still missing changes as it only completes a run with a minimal
> number of dependencies but open for feedback
>
> Requires pkgsrc packages for gmake, perl, bash and curl and completes a run
>
>   $ gmake SHELL_PATH=/usr/pkg/bin/bash NO_PYTHON=1 CURL_DIR=/usr/pkg test

This looks good to me, and fixes most of the issues I've encountered on
NetBSD recently. See
https://gitlab.com/git-vcs/git-gitlab-ci/blob/9337ed6f99e3780d1d8dc087dff688f5a68805a4/ci/gitlab/run-on-gcc-farm.sh#L228-239
and https://gitlab.com/git-vcs/git-ci/-/jobs/125476939