Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use command -v instead which command #3154

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kloczek
Copy link

@kloczek kloczek commented Feb 2, 2024

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

Copy link

openshift-ci bot commented Feb 2, 2024

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.

Minimize build requirement.
Use standard POSIX sh `command -v` instead separated external which command.

Fixes ostreedev#2825

Signed-off-by: Tomasz Kłoczko <[email protected]>
@ericcurtin
Copy link
Collaborator

ericcurtin commented Feb 3, 2024

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
Copy link
Author

kloczek commented Feb 3, 2024

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
Copy link
Collaborator

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

@jlebon
Copy link
Member

jlebon commented Feb 5, 2024

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
Copy link
Author

kloczek commented Feb 5, 2024

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
Copy link
Member

jlebon commented Feb 5, 2024

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

@ericcurtin
Copy link
Collaborator

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
Copy link
Collaborator

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

@ericcurtin
Copy link
Collaborator

ericcurtin commented Feb 5, 2024

bindatafiles="bash true ostree"

He meant change:

bindatafiles="bash true ostree"

to

bindatafiles="bash yes ostree"

which would likely fix this just fine.

@kloczek
Copy link
Author

kloczek commented Feb 6, 2024

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
Copy link
Collaborator

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
Copy link
Member

dbnicholson commented Feb 6, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link

dvzrv commented Feb 20, 2024

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

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)

pkg_install sudo which attr fuse bison \

pkg_install sudo which attr fuse strace \

@kloczek
Copy link
Author

kloczek commented Feb 20, 2024

@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
Copy link

dvzrv commented Feb 20, 2024

[..] 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! :)

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

@ericcurtin
Copy link
Collaborator

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
Copy link
Author

kloczek commented Feb 20, 2024

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
Copy link
Collaborator

ericcurtin commented Feb 20, 2024

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

@kloczek
Copy link
Author

kloczek commented Feb 21, 2024

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
Copy link
Collaborator

ericcurtin commented Feb 26, 2024

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
Copy link

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
Copy link
Author

kloczek commented Feb 26, 2024

This PR is ONLY about NUMBER of build dependencies.

@dbnicholson
Copy link
Member

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
Copy link
Member

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 cgwalters added triaged This issue has been evaluated and is valid reward/small This is a minor fix or cleanup difficulty/low Not extremely difficult needs-work/ci-failure Legitimate CI failure area/tests Changes to the tests (or CI flow) labels Feb 26, 2024
@cgwalters
Copy link
Member

/ok-to-test

@cgwalters
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tests Changes to the tests (or CI flow) difficulty/low Not extremely difficult needs-work/ci-failure Legitimate CI failure ok-to-test reward/small This is a minor fix or cleanup triaged This issue has been evaluated and is valid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2023.1: test suite is failing
7 participants