[PATCH 0/8] portability odds and ends

[PATCH 0/8] portability odds and ends

[Eric Wong]

Nothing major, just a few things I noticed while working
on various *BSDs.

Eric Wong (8):
  ci/run: favor `make check' as the default target
  drop GNU nproc(1) support in favor of getconf(1)
  ipc: assume SOCK_SEQPACKET exists
  rename t/run.perl to xt/check-run
  makefile: avoid needless use of double-colon rules
  pop3d: split @FLOCK into $FLOCK_TMPL and @FLOCK_ORDER
  install/deps: more fixes
  makefile: avoid non-POSIX which(1)

 Documentation/include.mk     | 10 ++++++----
 MANIFEST                     |  2 +-
 Makefile.PL                  | 31 +++++++++++++++++--------------
 ci/run.sh                    |  5 +++--
 install/deps.perl            | 17 ++++++++++++-----
 lib/PublicInbox/IPC.pm       | 22 +++++++++-------------
 lib/PublicInbox/POP3D.pm     | 16 ++++++++--------
 t/nntpd.t                    |  2 +-
 t/run.perl => xt/check-run.t |  4 ++--
 9 files changed, 59 insertions(+), 50 deletions(-)
 rename t/run.perl => xt/check-run.t (98%)

[PATCH 1/8] ci/run: favor `make check' as the default target

[Eric Wong]

While parallelism isn't always a good thing, `make check'
has more verbose output to help us track down occasionally
failing tests.
---
 ci/run.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ci/run.sh b/ci/run.sh
index 54563f1c..93790269 100755
--- a/ci/run.sh
+++ b/ci/run.sh
@@ -14,11 +14,12 @@ fi
 NPROC=${NPROC-$({ getconf _NPROCESSORS_ONLN || getconf NPROCESSORS_ONLN ||
 	gnproc || nproc || echo 2; } 2>/dev/null)}
 
+TEST_JOBS=${TEST_JOBS-1}
 $PERL -w ci/profiles.perl | while read args
 do
 	$DO $SUDO $PERL -w install/deps.perl -y --allow-remove $args
 	$DO $PERL Makefile.PL
 	$DO $MAKE -j${BUILD_JOBS-$NPROC}
-	$DO $MAKE -j${TEST_JOBS-1} ${TEST_TARGET-test}
+	$DO $MAKE ${TEST_TARGET-check} N=${N-$TEST_JOBS}
 	$DO $MAKE clean >/dev/null
 done

[PATCH 2/8] drop GNU nproc(1) support in favor of getconf(1)

[Eric Wong]

`getconf NPROCESSORS_ONLN' will succeed on GNU/Linux systems
anyways; and the non-underscore-prefixed invocation works fine
on all BSD flavors tested.

Thus the `nproc' and `gnproc' attempts will never be reached.
The only downside is we lose the ability to account for CPU
affinity, but that's probably not an issue since CPU affinity
(AFAIK) isn't a commonly-used feature.
---
 Makefile.PL            |  4 ++--
 ci/run.sh              |  2 +-
 lib/PublicInbox/IPC.pm | 11 ++++-------
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/Makefile.PL b/Makefile.PL
index 5b7914dc..97e00395 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -199,8 +199,8 @@ WriteMakefile(
 );
 
 sub MY::postamble {
-	my $N = (`{ getconf _NPROCESSORS_ONLN || getconf NPROCESSORS_ONLN ||
-		gnproc || nproc; } 2>/dev/null` || 1);
+	my $N = (`{ getconf _NPROCESSORS_ONLN ||
+		getconf NPROCESSORS_ONLN; } 2>/dev/null` || 1);
 	$N += 1; # account for sleeps in some tests (and makes an IV)
 	<<EOF;
 PROVE = prove
diff --git a/ci/run.sh b/ci/run.sh
index 93790269..bd1d8a4d 100755
--- a/ci/run.sh
+++ b/ci/run.sh
@@ -12,7 +12,7 @@ then
 	$DO $MAKE clean >/dev/null
 fi
 NPROC=${NPROC-$({ getconf _NPROCESSORS_ONLN || getconf NPROCESSORS_ONLN ||
-	gnproc || nproc || echo 2; } 2>/dev/null)}
+		echo 2; } 2>/dev/null)}
 
 TEST_JOBS=${TEST_JOBS-1}
 $PERL -w ci/profiles.perl | while read args
diff --git a/lib/PublicInbox/IPC.pm b/lib/PublicInbox/IPC.pm
index 39021f42..fa084795 100644
--- a/lib/PublicInbox/IPC.pm
+++ b/lib/PublicInbox/IPC.pm
@@ -464,16 +464,13 @@ sub detect_nproc () {
 	my $n = $NPROCESSORS_ONLN{$^O};
 	return POSIX::sysconf($n) if defined $n;
 
-	# getconf(1) is POSIX, but *NPROCESSORS* vars are not
+	# getconf(1) is POSIX, but *NPROCESSORS* vars are not even if
+	# glibc, {Free,Net,Open}BSD all support them.
 	for (qw(_NPROCESSORS_ONLN NPROCESSORS_ONLN)) {
 		`getconf $_ 2>/dev/null` =~ /^(\d+)$/ and return $1;
 	}
-	for my $nproc (qw(nproc gnproc)) { # GNU coreutils nproc
-		`$nproc 2>/dev/null` =~ /^(\d+)$/ and return $1;
-	}
-
-	# should we bother with `sysctl hw.ncpu`?  Those only give
-	# us total processor count, not online processor count.
+	# note: GNU nproc(1) checks CPU affinity, which is nice but
+	# isn't remotely portable
 	undef
 }
 

[PATCH 3/8] ipc: assume SOCK_SEQPACKET exists

[Eric Wong]

The rest of our code does, and we haven't encountered a platform
we'd care about without it.
---
 lib/PublicInbox/IPC.pm | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/IPC.pm b/lib/PublicInbox/IPC.pm
index fa084795..9388befd 100644
--- a/lib/PublicInbox/IPC.pm
+++ b/lib/PublicInbox/IPC.pm
@@ -16,9 +16,8 @@ use PublicInbox::DS qw(awaitpid);
 use PublicInbox::Spawn;
 use PublicInbox::OnDestroy;
 use PublicInbox::WQWorker;
-use Socket qw(AF_UNIX SOCK_STREAM);
+use Socket qw(AF_UNIX SOCK_STREAM SOCK_SEQPACKET);
 my $MY_MAX_ARG_STRLEN = 4096 * 33; # extra 4K for serialization
-my $SEQPACKET = eval { Socket::SOCK_SEQPACKET() }; # portable enough?
 our @EXPORT_OK = qw(ipc_freeze ipc_thaw nproc_shards);
 my ($enc, $dec);
 # ->imports at BEGIN turns sereal_*_with_object into custom ops on 5.14+
@@ -374,7 +373,7 @@ sub wq_nonblock_do { # always async
 sub _wq_worker_start {
 	my ($self, $oldset, $fields, $one, @cb_args) = @_;
 	my ($bcast1, $bcast2);
-	$one or socketpair($bcast1, $bcast2, AF_UNIX, $SEQPACKET, 0) or
+	$one or socketpair($bcast1, $bcast2, AF_UNIX, SOCK_SEQPACKET, 0) or
 							die "socketpair: $!";
 	my $seed = rand(0xffffffff);
 	my $pid = fork // die "fork: $!";
@@ -409,11 +408,11 @@ sub _wq_worker_start {
 # starts workqueue workers if Sereal or Storable is installed
 sub wq_workers_start {
 	my ($self, $ident, $nr_workers, $oldset, $fields, @cb_args) = @_;
-	($send_cmd && $recv_cmd && defined($SEQPACKET)) or return;
+	($send_cmd && $recv_cmd) or return;
 	return if $self->{-wq_s1}; # idempotent
 	$self->{-wq_s1} = $self->{-wq_s2} = undef;
-	socketpair($self->{-wq_s1}, $self->{-wq_s2}, AF_UNIX, $SEQPACKET, 0) or
-		die "socketpair: $!";
+	socketpair($self->{-wq_s1}, $self->{-wq_s2}, AF_UNIX, SOCK_SEQPACKET, 0)
+		or die "socketpair: $!";
 	$self->ipc_atfork_prepare;
 	$nr_workers //= $self->{-wq_nr_workers}; # was set earlier
 	my $sigset = $oldset // PublicInbox::DS::block_signals();

[PATCH 4/8] rename t/run.perl to xt/check-run

[Eric Wong]

This allows us to get rid of some duplication in our Makefile
---
 MANIFEST                     |  2 +-
 Makefile.PL                  | 15 +++++++++------
 t/nntpd.t                    |  2 +-
 t/run.perl => xt/check-run.t |  4 ++--
 4 files changed, 13 insertions(+), 10 deletions(-)
 rename t/run.perl => xt/check-run.t (98%)

diff --git a/MANIFEST b/MANIFEST
index 2fe5dd17..7dba3836 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -577,7 +577,6 @@ t/reindex-time-range.t
 t/rename_noreplace.t
 t/replace.t
 t/reply.t
-t/run.perl
 t/search-amsg.eml
 t/search-thr-index.t
 t/search.t
@@ -621,6 +620,7 @@ t/xap_helper.t
 t/xcpdb-reshard.t
 version-gen.perl
 xt/check-debris.t
+xt/check-run.t
 xt/cmp-msgstr.t
 xt/create-many-inboxes.t
 xt/eml_check_limits.t
diff --git a/Makefile.PL b/Makefile.PL
index 97e00395..9dc18b5c 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -235,13 +235,16 @@ check-each :: pure_all
 	\$(EATMYDATA) \$(PROVE) --state=save -bvw -j\$(N)
 	-@\$(check_manifest)
 
-# this relies "--state=save" in check-each for best performance
-check-run :: pure_all check-man
-	\$(EATMYDATA) \$(PROVE) -bvw t/run.perl :: -j\$(N)
-	-@\$(check_manifest)
+# check-run relies "--state=save" in check-each for best performance
+check-run :: check-man
+
+# n.b. while `-' isn't specified as an allowed make(1posix) macro name,
+# GNU and *BSD both allow it.
+check-run_T_ARGS = -j\$(N)
 
-check-debris :: pure_all
-	PERL5LIB="\$\$PWD"/blib/lib \$(PROVE) -bvw xt/\$@.t
+check-debris check-run :: pure_all
+	\$(EATMYDATA) \$(PROVE) -bvw xt/\$@.t :: \$(\$\@_T_ARGS)
+	-@\$(check_manifest)
 
 check :: check-each
 
diff --git a/t/nntpd.t b/t/nntpd.t
index 9a7efd86..ffe0fd8c 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -330,7 +330,7 @@ Date: Fri, 02 Oct 1993 00:00:00 +0000
 	SKIP: {
 		if ($INC{'Search/Xapian.pm'} || $INC{'Xapian.pm'} &&
 				($ENV{TEST_RUN_MODE} // 2)) {
-			skip 'Xapian.pm pre-loaded (by t/run.perl?)', 1;
+			skip 'Xapian.pm pre-loaded (by xt/check-run.t?)', 1;
 		}
 		$lsof or skip 'lsof missing', 1;
 		my @of = xqx([$lsof, '-p', $td->{pid}], undef, $noerr);
diff --git a/t/run.perl b/xt/check-run.t
similarity index 98%
rename from t/run.perl
rename to xt/check-run.t
index 9c8129d5..6eefcb7d 100755
--- a/t/run.perl
+++ b/xt/check-run.t
@@ -8,8 +8,8 @@
 #
 # *.t files run by this should not rely on global state.
 #
-# Usage: $PERL -I lib -w t/run.perl -j4
-# Or via prove(1): prove -lvw t/run.perl :: -j4
+# Usage: $PERL -I lib -w xt/check-run.t -j4
+# Or via prove(1): prove -lvw xt/check-run.t :: -j4
 use v5.12;
 use IO::Handle; # ->autoflush
 use PublicInbox::TestCommon;

[PATCH 5/8] makefile: avoid needless use of double-colon rules

[Eric Wong]

This non-portable construct isn't needed for our own rules.
I'm understanding them correctly, they have different
semantics between *BSDs and GNU make.
---
 Documentation/include.mk |  8 ++++----
 Makefile.PL              | 14 +++++++-------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/include.mk b/Documentation/include.mk
index 4395e4f4..eb65454a 100644
--- a/Documentation/include.mk
+++ b/Documentation/include.mk
@@ -47,14 +47,14 @@ install-man: man
 
 doc_install :: install-man
 
-check :: check-man
+check : check-man
 check_man = $(AWK) \
 	'{gsub(/\b./,"")}$$0 !~ /\.onion/&&length>80{print;e=1}END{exit(e)}' \
 	>&2
 
-check-man :: $(check_80)
+check-man : $(check_80)
 
-check-lexgrog :: $(check_lexgrog)
+check-lexgrog : $(check_lexgrog)
 
 all :: $(docs)
 
@@ -86,7 +86,7 @@ NEWS NEWS.atom NEWS.html : $(news_deps)
 	$(PERL) -I lib -w Documentation/mknews.perl $@ $(RELEASES)
 
 # check for internal API changes:
-check :: NEWS .NEWS.atom.check NEWS.html
+check : NEWS .NEWS.atom.check NEWS.html
 
 .NEWS.atom.check: NEWS.atom
 	$(XMLSTARLET) val NEWS.atom || \
diff --git a/Makefile.PL b/Makefile.PL
index 9dc18b5c..81992e46 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -213,7 +213,7 @@ $VARS
 -include Documentation/include.mk
 $TGTS
 
-check-man :: $check_lexgrog$warn_no_pod
+check-man : $check_lexgrog$warn_no_pod
 
 # syntax checks are currently GNU make only:
 %.syntax :: %
@@ -231,24 +231,24 @@ check-manifest : MANIFEST
 	\$(check_manifest)
 
 # the traditional way running per-*.t processes:
-check-each :: pure_all
+check-each : pure_all
 	\$(EATMYDATA) \$(PROVE) --state=save -bvw -j\$(N)
 	-@\$(check_manifest)
 
 # check-run relies "--state=save" in check-each for best performance
-check-run :: check-man
+check-run : check-man
 
 # n.b. while `-' isn't specified as an allowed make(1posix) macro name,
 # GNU and *BSD both allow it.
 check-run_T_ARGS = -j\$(N)
 
-check-debris check-run :: pure_all
+check-debris check-run : pure_all
 	\$(EATMYDATA) \$(PROVE) -bvw xt/\$@.t :: \$(\$\@_T_ARGS)
 	-@\$(check_manifest)
 
-check :: check-each
+check : check-each
 
-lib/PublicInbox/UserContent.pm :: contrib/css/216dark.css
+lib/PublicInbox/UserContent.pm : contrib/css/216dark.css
 	\$(PERL) -I lib \$@ \$?
 
 # Ensure new .pm files will always be installed by updating
@@ -269,7 +269,7 @@ symlink-install : lib/PublicInbox.pm
 		ln -sf "\$\$lei" \$\$(basename "\$\$x"); \\
 	done
 
-pure_all :: lib/PublicInbox.pm
+pm_to_blib : lib/PublicInbox.pm
 lib/PublicInbox.pm : FORCE
 	VERSION=\$(VERSION) \$(PERL) -w ./version-gen.perl
 

[PATCH 6/8] pop3d: split @FLOCK into $FLOCK_TMPL and @FLOCK_ORDER

[Eric Wong]

This improves readability since we don't have to subscript
the array for use with the `pack' perlop.
---
 lib/PublicInbox/POP3D.pm | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/PublicInbox/POP3D.pm b/lib/PublicInbox/POP3D.pm
index 580a26d3..bee668fc 100644
--- a/lib/PublicInbox/POP3D.pm
+++ b/lib/PublicInbox/POP3D.pm
@@ -13,7 +13,7 @@ use PublicInbox::POP3;
 use PublicInbox::Syscall;
 use File::Temp 0.19 (); # 0.19 for ->newdir
 use Fcntl qw(F_SETLK F_UNLCK F_WRLCK SEEK_SET);
-my @FLOCK;
+my ($FLOCK_TMPL, @FLOCK_ORDER);
 # are all BSDs the same "struct flock"? tested Free+Net+Open...
 if ($^O eq 'linux' || $^O =~ /bsd/) {
 	require Config;
@@ -26,15 +26,15 @@ if ($^O eq 'linux' || $^O =~ /bsd/) {
 
 	if (defined($off_t)) {
 		if ($^O eq 'linux') {
-			@FLOCK = ("ss\@8$off_t$off_t\@32",
-				qw(l_type l_whence l_start l_len));
+			$FLOCK_TMPL = "ss\@8$off_t$off_t\@32";
+			@FLOCK_ORDER = qw(l_type l_whence l_start l_len);
 		} elsif ($^O =~ /bsd/) { # @32 may be enough
-			@FLOCK = ("${off_t}${off_t}lss\@256",
-				qw(l_start l_len l_pid l_type l_whence));
+			$FLOCK_TMPL = "${off_t}${off_t}lss\@256";
+			@FLOCK_ORDER = qw(l_start l_len l_pid l_type l_whence);
 		}
 	}
 }
-@FLOCK or eval { require File::FcntlLock } or
+@FLOCK_ORDER or eval { require File::FcntlLock } or
 	die "File::FcntlLock required for POP3 on $^O: $@\n";
 
 sub new {
@@ -141,9 +141,9 @@ sub _setlk ($%) {
 	my ($self, %lk) = @_;
 	$lk{l_pid} = 0; # needed for *BSD
 	$lk{l_whence} = SEEK_SET;
-	if (@FLOCK) {
+	if (@FLOCK_ORDER) {
 		fcntl($self->{txn_fh}, F_SETLK,
-			pack($FLOCK[0], @lk{@FLOCK[1..$#FLOCK]}));
+			pack($FLOCK_TMPL, @lk{@FLOCK_ORDER}));
 	} else {
 		my $fs = File::FcntlLock->new(%lk);
 		$fs->lock($self->{txn_fh}, F_SETLK);

[PATCH 7/8] install/deps: more fixes

[Eric Wong]

We need to mark *-dev packages as optional, and ignore
dependency-only packages in the `optional' field.

Furthermore, make the output less confusing when there's neither
packages to install nor remove; and avoid invoking `apt-get install'
with an empty package list.

This also fixes an OpenBSD-specific regression from commit 82990fb72dac
which made package removal a no-op.

Fixes: 82990fb72dac (install/deps: flesh out libgit2, SQLite, and Xapian packages)
---
 install/deps.perl | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/install/deps.perl b/install/deps.perl
index 900668c7..65547f9d 100755
--- a/install/deps.perl
+++ b/install/deps.perl
@@ -48,7 +48,8 @@ my $profiles = {
 		URI
 		), @test_essential ],
 
-	# everything optional for normal use
+	# Everything else is optional for normal use.  Only specify
+	# the minimum to pull in dependencies here:
 	optional => [ qw(
 		Date::Parse
 		BSD::Resource
@@ -63,11 +64,14 @@ my $profiles = {
 		Xapian
 		curl
 		highlight.pm
-		libxapian
-		pkg-config
+		libgit2-dev
+		libxapian-dev
 		sqlite3
 		xapian-tools
 		) ],
+		# no pkg-config, libsqlite3, libxapian, libz, etc. since
+		# they'll get pulled in lib*-dev, DBD::SQlite and
+		# Xapian(.pm)  respectively
 
 	# optional developer stuff
 	devtest => [ qw(
@@ -279,6 +283,8 @@ my (%add, %rm); # uniquify lists
 		!$add{$_} && !$rm{$_}++ && $INST_CHECK->($_)
 	} @pkg_remove : ();
 
+(@pkg_remove || @pkg_install) or warn "# no packages to install nor remove\n";
+
 # OS-specific cleanups appreciated
 if ($pkg_fmt eq 'deb') {
 	my @apt_opt = qw(-o APT::Install-Recommends=false
@@ -288,7 +294,7 @@ if ($pkg_fmt eq 'deb') {
 		@pkg_install,
 		# apt-get lets you suffix a package with "-" to
 		# remove it in an "install" sub-command:
-		map { "$_-" } @pkg_remove);
+		map { "$_-" } @pkg_remove) if (@pkg_remove || @pkg_install);
 	root('apt-get', @apt_opt, qw(autoremove)) if $opt->{'allow-remove'};
 } elsif ($pkg_fmt eq 'pkg') { # FreeBSD
 	my @pkg_opt = $opt->{yes} ? qw(-y) : ();
@@ -309,9 +315,10 @@ if ($pkg_fmt eq 'deb') {
 	root(qw(yum install), @pkg_opt, @pkg_install) if @pkg_install;
 } elsif ($pkg_fmt eq 'pkg_add') { # OpenBSD
 	my @pkg_opt = $opt->{yes} ? qw(-I) : (); # -I means non-interactive
-	root(qw(pkg_delete -a), @pkg_opt); # autoremove unspecified
+	root(qw(pkg_delete), @pkg_opt, @pkg_remove) if @pkg_remove;
 	@pkg_install = map { "$_--" } @pkg_install; # disambiguate w3m
 	root(qw(pkg_add), @pkg_opt, @pkg_install) if @pkg_install;
+	root(qw(pkg_delete -a), @pkg_opt) if $opt->{'allow-remove'};
 } else {
 	die "unsupported package format: $pkg_fmt\n";
 }

[PATCH 8/8] makefile: avoid non-POSIX which(1)

[Eric Wong]

which(1) isn't in POSIX so it's perhaps less likely to be
available (although I don't think I've noticed a system
without it in decades).  So replace it with the POSIX
`command -v', even though everyone seems to use which...

Add a note about `lexgrog', too, since I'm not sure if it's
packaged for various *BSDs.
---
 Documentation/include.mk | 2 ++
 Makefile.PL              | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/include.mk b/Documentation/include.mk
index eb65454a..86851376 100644
--- a/Documentation/include.mk
+++ b/Documentation/include.mk
@@ -6,6 +6,8 @@ RSYNC = rsync
 RSYNC_DEST = public-inbox.org:/srv/public-inbox/
 AWK = awk
 MAN = man
+
+# part of `man-db' on Debian, not sure about other distros
 LEXGROG = lexgrog
 
 # this is "xml" on FreeBSD and maybe some other distros:
diff --git a/Makefile.PL b/Makefile.PL
index 81992e46..8bd39e6c 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -126,7 +126,7 @@ my %man3 = map {; # semi-colon tells Perl this is a BLOCK (and not EXPR)
 	"lib/PublicInbox/$_" => "blib/man3/PublicInbox::$mod.\$(MAN3EXT)"
 } qw(Git.pm Import.pm WWW.pod SaPlugin/ListMirror.pod);
 my $warn_no_pod = @no_pod ? "\n\t\@echo W: missing .pod: @no_pod\n" : '';
-chomp(my $lexgrog = `which lexgrog 2>/dev/null`);
+chomp(my $lexgrog = `command -v lexgrog 2>/dev/null`);
 my $check_lexgrog = $lexgrog ? 'check-lexgrog' : '';
 
 WriteMakefile(