use command -v instead which command

[kloczek]

Minimize build requirement.
Use standard POSIX sh command -v instead separader external which comman. Fixes #2825

[openshift-ci]

Hi @kloczek. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ericcurtin]

This is one of those PRs that you click into and expect it's gonna be an easy review and merge... And then it's not...

Maybe we can do some kind of an or, use which or command -v ?

I also notice in one of these files the shell isn't specified in the top line #!/bin/bash... Don't know if that's contributing to the build failures...

[kloczek]

command it is internal command of the POXIX sh. From https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html

DESCRIPTION
The command utility shall cause the shell to treat the arguments as a simple command, suppressing the shell function lookup that is described in [Command Search and Execution](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01), item 1b.

If the command_name is the same as the name of one of the special built-in utilities, the special properties in the enumerated list at the beginning of [Special Built-In Utilities](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_14) shall not occur. In every other respect, if command_name is not the name of a function, the effect of command (with no options) shall be the same as omitting command.

When the -v or -V option is used, the command utility shall provide information concerning how a command name is interpreted by the shell.

OPTIONS
The command utility shall conform to XBD [Utility Syntax Guidelines](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02).

The following options shall be supported:

-p
Perform the command search using a default value for PATH that is guaranteed to find all of the standard utilities.
-v
Write a string to standard output that indicates the pathname or command that will be used by the shell, in the current shell execution environment (see [Shell Execution Environment](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_12)), to invoke command_name, but do not invoke command_name.
Utilities, regular built-in utilities, command_names including a <slash> character, and any implementation-defined functions that are found using the PATH variable (as described in [Command Search and Execution](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01)), shall be written as absolute pathnames.

Shell functions, special built-in utilities, regular built-in utilities not associated with a PATH search, and shell reserved words shall be written as just their names.

An alias shall be written as a command line that represents its alias definition.

Otherwise, no output shall be written and the exit status shall reflect that the name was not found.

[ericcurtin]

I hear ya, I'm just wondering why the builds are failing, does this work on all the builds?

[jlebon]

Probably because true is shadowed by a bash built-in. It'd probably work to just pick some other coreutils binary instead, e.g. yes.

[kloczek]

Probably because true is shadowed by a bash built-in. It'd probably work to just pick some other coreutils binary instead, e.g. yes.

Hmm .. but his PR is not changing anything around use true or yes command 😋

[jlebon]

We iterate over bindatafiles in the delta tests, and it contains true.

[ericcurtin]

Good spot @jlebon ! Changing:

bindatafiles="bash true ostree"

to

bindatafiles="bash /usr/bin/true ostree"

should work. Although leaving which as a build dependency would be fine...

[ericcurtin]

We could just leave true out of this list also, I think it's just copying random files for a test.

[ericcurtin]

bindatafiles="bash true ostree"

He meant change:

bindatafiles="bash true ostree"

to

bindatafiles="bash yes ostree"

which would likely fix this just fine.

[kloczek]

to

bindatafiles="bash yes ostree"

which would likely fix this just fine.

In above there is nothing bash specific.
Why not 'bindatafiles="sh yes ostree"'? 🤔

[ericcurtin]

to

bindatafiles="bash yes ostree"

which would likely fix this just fine.

In above there is nothing bash specific. Why not 'bindatafiles="sh yes ostree"'? 🤔

I think if you take preference it's fine as long as it's reasonably portable. /bin/bash is a build dependency though, it's in the shebang line of these scripts.

[dbnicholson]

There's certainly no harm in this, but I'm fairly positive that the tests require bash. When I look over libtest.sh and libtest-core.sh, it mostly looks POSIX compliant to me. However, the main developers are mostly on Fedora where you have to go out of your way to use a non-bash POSIX shell. Unless something like shellcheck is used to ensure proper syntax, it seems likely that POSIX compliance will easily get broken. Since bash is already explicitly used in several parts of the tests, I think the best thing to do is ensure that bash is the shell used for all tests.

[dbnicholson]

There's certainly no harm in this, but I'm fairly positive that the tests require bash. When I look over libtest.sh and libtest-core.sh, it mostly looks POSIX compliant to me. However, the main developers are mostly on Fedora where you have to go out of your way to use a non-bash POSIX shell. Unless something like shellcheck is used to ensure proper syntax, it seems likely that POSIX compliance will easily get broken. Since bash is already explicitly used in several parts of the tests, I think the best thing to do is ensure that bash is the shell used for all tests.

Oh, but it does appear that bash is used basically everywhere. The problem is that which is not a bash builtin (I thought it was). So, I think this PR completely makes sense as it's not a POSIX/non-POSIX issue but rather removing the unspecified dependency on the which program.

[dbnicholson]

to

bindatafiles="bash yes ostree"

which would likely fix this just fine.

In above there is nothing bash specific. Why not 'bindatafiles="sh yes ostree"'? 🤔

I think if you take preference it's fine as long as it's reasonably portable. /bin/bash is a build dependency though, it's in the shebang line of these scripts.

In this case, I think the better fix is not to use command -v since the test wants actual files on disk, not shell builtins. In that case, since the test is already using bash, type -P is a better replacement for what which was being used for. See https://www.gnu.org/software/bash/manual/html_node/Bash-Builtins.html#index-type.

[dvzrv]

Hi! 👋
I'm packaging this project for Arch Linux and would also like to see which be replaced with the shell builtin.

@kloczek there are further uses of which that can and probably should be adapted, such as

ostree/src/boot/grub2/grub2-15_ostree

Line 20 in 695a52a

if ! which ostree >/dev/null 2>/dev/null; then

The above in particular is problematic, as it breaks on enduser systems if which is not installed: https://gitlab.archlinux.org/archlinux/packaging/packages/ostree/-/issues/1

Adapting the following would rid this project of which as build dependency as well:

ostree/autogen.sh

Lines 9 to 19 in 695a52a

	AUTORECONF=`which autoreconf`
	if test -z $AUTORECONF; then
	echo "*** No autoreconf found, please install it ***"
	exit 1
	fi
	set -e
	mkdir -p m4
	GTKDOCIZE=$(which gtkdocize 2>/dev/null || true)

ostree/ci/flatpak.sh

Line 33 in 695a52a

pkg_install sudo which attr fuse bison \

ostree/ci/installdeps.sh

Line 20 in 695a52a

pkg_install sudo which attr fuse strace \

[kloczek]

@kloczek there are further uses of which that can and probably should be adapted, such as

[..]
Generally speaking nothing in those scripts needs to know exact locations of the commands like yes or auoreconf.
Checking that location before execution is just nor relevant. In other it is possible to remove more such checks code and still everything will be perfectly fine.
Whole ostree/autogen.sh content can be replaced by single line in that script with just autoreconf -fiv.

Bash is generally compatible with POSIX sh (+/- how it is handled loops body which in case of ostree is as well not relevant) so is if someone will be using symlink bash->sh this PR will change nothing as well.

[dvzrv]

[..] Generally speaking nothing in those scripts needs to know exact locations of the commands like yes or auoreconf. Checking that location before execution is just nor relevant. In other it is possible to remove more such checks code and still everything will be perfectly fine. Whole ostree/autogen.sh content can be replaced by single line in that script with just autoreconf -fiv.

Yes, that's why I wrote "adapted".
How this takes place is not really of my concern, as long as the use of which is gone afterwards :)

While it would be great to remove which from the build system dependencies, I am definitely way more interested in removing it from the runtime dependencies.

So if you could adjust the grub related occurrence in this PR as well, that would be ace! :)

ostree/src/boot/grub2/grub2-15_ostree

Line 20 in 695a52a

if ! which ostree >/dev/null 2>/dev/null; then

[ericcurtin]

If we just use type -P like @dbnicholson said, this is a very easy PR to merge btw...

I'm not sure it's worth delving too deeply into POSIX, bash is a dependency and we only support one Unix-like platform, Linux.

[kloczek]

If we just use type -P like @dbnicholson said, this is a very easy PR to merge btw...

type -P is not part of the POSIX sh spec.

[ericcurtin]

I don't think POSIX compliance is a goal of ostree tbh, bash is a dependency, type -P is part of bash

[kloczek]

I don't think POSIX compliance is a goal of ostree tbh, bash is a dependency, type -P is part of bash

What is the gal of ostree may be orthogonal to targets of the distributions using ostree.
Bash is not the fastest shell and time on build systems has real costs as today distros consists of tenths of thousands packages.
Please keep those bits in mind ..

[ericcurtin]

I'm not sure if bash is used in any performance-critical runtime path on ostree, as a build/test time dependency it should be ok on most platforms. Installing extra dependancies for build time is normally ok.

That said if there are one or two places bash is used during actual runtime in final deliverable packages I would be a fan of rewriting those shell scripts from the perspective of being friendly to distributions that don't distribute GPLv3 code, maybe it would make sense to be POSIX compatible, multi-shell compatible, portable in these cases, where they exist.

But these are tests. I think you could write these tests to be POSIX friendly if you wanted to, but this PR doesn't work, it's failing the build.

[Conan-Kudo]

The difference in performance among various sh implementations just does not matter anymore. It's so measurably small that it's more important to align on expectations in a project. For OSTree, that's a Unix-ish system that has GNU-ish userland and offers Bash.

[kloczek]

This PR is ONLY about NUMBER of build dependencies.

[dbnicholson]

Bash is generally compatible with POSIX sh (+/- how it is handled loops body which in case of ostree is as well not relevant) so is if someone will be using symlink bash->sh this PR will change nothing as well.

This project uses bash in several places and expects to use the features of bash. That's why all of those scripts start with #!/bin/bash. Sorry, but if /bin/bash isn't actually bash, that's not supported. I understand a goal for you is to rid yourself of bash, but it's not a goal of this project. The issue you're addressing with this PR is legitimate, but it's not about bash vs POSIX shell.

[cgwalters]

Agree with Dan above. So no objections to merging from me, but the test suite has to work, and this currently breaks it:

 ERROR: tests/test-delta.sh
==========================
....
+ cp true files
cp: cannot stat 'true': No such file or directory

It seems there's not an option to get the full path; what this test is doing is inherently kind of hacky of course...

[cgwalters]

/ok-to-test

[cgwalters]

@dvzrv

While it would be great to remove which from the build system dependencies, I am definitely way more interested in removing it from the runtime dependencies.

Yes, but if you really want to trim that, you should be trying to set bootloader.backend=none and having ostree just write BLS files that the bootloader interprets at runtime - then you don't need this file at all.