Failure in checks/colon-delimited-var.fish on OpenIndiana

[Mno-hime]

fish 3.1 beta1 test suite compiled with GCC 6.5 fails on OpenIndiana with:

Testing file checks/colon-delimited-var.fish ... Failure in checks/colon-delimited-var.fish:

  The CHECK on line 8 wants:
    a:.:b

  which failed to match line stdout:1:
    /usr/xpg4/bin:a:.:b

  when running command:
    env PATH="a::b" CDPATH="d::e" MANPATH="x::y" ../test/root/bin/fish checks/colon-delimited-var.fish

[krobelus]

Could this be due to the vendor directories for configuration snippets?
Some file in $vendor_confdirs in share/config.fish could add the PATH entry.

What changed is that /usr/local/share/fish/vendor_conf.d/*.fish are included by default, even if running from a different prefix (like the tests do).

[zanchey]

Nope, this is added deliberately in share/config.fish:

fish-shell/share/config.fish

Lines 119 to 129 in b313ba5

	# This is a Solaris-specific test to modify the PATH so that
	# Posix-conformant tools are used by default. It is separate from the
	# other PATH code because this directory needs to be prepended, not
	# appended, since it contains POSIX-compliant replacements for various
	# system utilities.
	#
	if test -d /usr/xpg4/bin
	not contains -- /usr/xpg4/bin $PATH
	and set PATH /usr/xpg4/bin $PATH
	end

From all the way back in 2006! 8a2846e

I actually think we should drop these kind of platform hacks but at the very least it should be skipped when the tests are running.

[faho]

I actually think we should drop these kind of platform hacks

The one reason I can see for this is that we used to call external tools quite a bit, and the solaris ones are... different, so this would help. But since this was introduced we've reduced our dependence on external tools quite a bit (e.g. via string and math), so this has even less of a reason for existing.

So: Yes, let's remove it, but preferably for 3.2. For 3.1 I'd just accept the failure - turning the test off doesn't seem like it'd help much.

[Mno-hime]

Just FYI, I run the build and test with GNU utilities taking precedence in the $PATH.

Anyway, I am happy to give patch a shot, when there's one.

[zanchey]

Something like this should work:

diff --git a/share/config.fish b/share/config.fish
index 400556832..6157c7c1c 100644
--- a/share/config.fish
+++ b/share/config.fish
@@ -123,7 +123,7 @@ end
 # system utilities.
 #

-if test -d /usr/xpg4/bin
+if begin; not set -q FISH_UNIT_TESTS_RUNNING; and test -d /usr/xpg4/bin; end
     not contains -- /usr/xpg4/bin $PATH
     and set PATH /usr/xpg4/bin $PATH
 end

(or just patch the test to use /usr/xpg4/bin as the test path, I suppose)

[faho]

Something like this should work:

TBH I'd rather just remove that code entirely:

diff --git a/share/config.fish b/share/config.fish
index 400556832..acba33c09 100644
--- a/share/config.fish
+++ b/share/config.fish
@@ -115,19 +115,6 @@ function : -d "no-op function"
     true
 end

-#
-# This is a Solaris-specific test to modify the PATH so that
-# Posix-conformant tools are used by default. It is separate from the
-# other PATH code because this directory needs to be prepended, not
-# appended, since it contains POSIX-compliant replacements for various
-# system utilities.
-#
-
-if test -d /usr/xpg4/bin
-    not contains -- /usr/xpg4/bin $PATH
-    and set PATH /usr/xpg4/bin $PATH
-end
-

Granted it's a bit late in the cycle, but it would be nice to know what, if anything, breaks.

[ridiculousfish]

Patching the test in 3.1, and removing the xpg4 check in master, seems sensible to me.

[zanchey]

If @Mno-hime can confirm that my patch works I'll do that.

[Mno-hime]

I run the @faho's change now, happy to test a 3.1-specific one too.

[zanchey]

@Mno-hime please see #6556 (comment)

[Mno-hime]

@zanchey That works for me too. Thanks!