In the past, before commit d323143c46, there was either had a
dummy 'return' statement or a self-documenting 'panic' that said that
the code should not be reached. Since neither golangci-lint nor
'go vet' likes those, a comment is the only option left.
Note that the core Go tools like 'go vet' [1], but also 'go lint' [2],
explicitly don't intend to add fine-grained configuration options,
including inline directives or pragmas, to silence specific warnings.
That's something golangci-lint offers [3], to the extent that it's
supported by its linters [4]. However, golangci-lint also uses 'go vet'
as one of those linters, so it's the same problem all over again.
Therefore, between the two extremes of leaving the code difficult to
read and using a very big hammer to disable a needlessly big chuck of
'go vet', a comment is the least worst option.
[1] https://github.com/golang/go/issues/17058https://github.com/golang/go/issues/18432
[2] https://github.com/golang/lint/issues/263
[3] https://golangci-lint.run/usage/false-positives/
[4] https://golangci-lint.run/usage/linters/
Fallout from d323143c46https://github.com/containers/toolbox/pull/1185
Using the word 'containerized' gives the false impression of heightened
security. As if it's a mechanism to run untrusted software in a
sandboxed environment without access to the user's private data (such as
$HOME), hardware peripherals (such as cameras and microphones), etc..
That's not what Toolbx is for.
Toolbx aims to offer an interactive command line environment for
development and troubleshooting the host operating system, without
having to install software on the host. That's all. It makes no
promise about security beyond what's already available in the usual
command line environment on the host that everybody is familiar with.
https://github.com/containers/toolbox/issues/1020
Mention that Toolbx is meant for system administrators to troubleshoot
the host operating system. The word 'debugging' is often used in the
context of software development, and hence most readers might not
interpret it as 'troubleshooting'.
https://github.com/containers/toolbox/pull/1182
Otherwise, every zsh instance on Fedora Kinoite and Silverblue was
running into:
/etc/profile.d/toolbox.sh:30: bad substitution
... because case modification with "${VARIANT_ID^}" is undefined in
POSIX shell [1], and doesn't work with Z shell.
Fedora Silverblue got its own PRETTY_NAME (and VARIANT and VARIANT_ID)
starting from Fedora 32 [2]. Therefore, it's better to use PRETTY_NAME
and let the downstream distributor of the host operating system decide
how it should be presented to the user, instead of coming up with a
custom string. eg., PRETTY_NAME isn't the same as "Fedora $VARIANT" on
Fedora Silverblue.
One nice side-effect of this is that while VARIANT and VARIANT_ID are
optional fields, PRETTY_NAME has a well-defined fallback value of
'Linux' [3]. This makes this a little less specific to Fedora Kinoite
and Silverblue.
The rest of the welcome text was reformatted to prevent it from getting
too wide depending on the contents of PRETTY_NAME.
Fallout from 3641a0032f
[1] https://www.shellcheck.net/wiki/SC3059
[2] https://pagure.io/workstation-ostree-config/c/c18ef957d11862d32f362722931dbfdf1f5beb0d
[3] https://www.freedesktop.org/software/systemd/man/os-release.htmlhttps://github.com/containers/toolbox/issues/1017
On a couple of occasions the relevant tests didn't get triggered because
some files weren't listed [1], and on another a commit forgot to update
the list of files [2].
The objective of the CI is to reduce stress for the maintainers, and
make it easy for contributors to find out if their changes work or not.
Missing tests don't help with that, and there's no need to optimize the
tests like this unless there's a real problem to be solved.
[1] Commit deca452b27
Commit 5c27d73021
[2] Commit b1743c4927
This reverts commit c28d902089.
https://github.com/containers/toolbox/pull/1168
Some OSTree based systems, such as Endless OS, don't ship with
/usr/lib/os-release, and the os-release(5) manual says [1]:
The file /etc/os-release takes precedence over /usr/lib/os-release.
Applications should check for the former, and exclusively use its data
if it exists, and only fall back to /usr/lib/os-release if it is
missing.
[1] https://www.freedesktop.org/software/systemd/man/os-release.htmlhttps://github.com/containers/toolbox/pull/692
This is a precursor to checking that higher valued exit codes from the
command running inside the container are retained, and commands like
test(1) can be used with 'toolbox run ...' in subsequent test cases.
https://github.com/containers/toolbox/pull/1163
Currently, some of the names of the tests were too long, and had
inconsistent and verbose wording. This made it difficult to look at
them and get a gist of all the scenarios being tested. The names are
like headings. They shouldn't be too long, should capture the primary
objective of the test and be consistent in their wording.
https://github.com/containers/toolbox/pull/1161
Currently, 'meson compile' and 'meson install' were being invoked from
pre-run playbooks. This meant that a genuine build failure from either
of those commands would be shown as a RETRY_LIMIT failure by the CI.
This was misleading. It made it look as if the failure was caused by
some transient networking problem or that the CI node was too slow due
to momentary heavy load, whereas the failure was actually due to a
problem in the Toolbx sources. A genuine problem in the sources should
be reflected as a FAILURE, not RETRY_LIMIT.
However, it's worth noting that 'meson compile' invokes 'go build',
which downloads all the Go modules required by the Toolbx sources. This
is worth retaining in the pre-run playbooks since it primarily depends
on Internet infrastructure beyond the Toolbx sources.
As a nice side-effect, the CI no longer gets mysteriously stuck like
this while the Go modules are being downloaded:
TASK [Build Toolbox]
ci-node-36 | ninja: Entering directory
`/home/zuul-worker/src/github.com/containers/toolbox/builddir'
...
ci-node-36 | [8/13] Generating doc/toolbox-rmi.1 with a custom command
ci-node-36 | [9/13] Generating doc/toolbox-run.1 with a custom command
ci-node-36 | [10/13] Generating doc/toolbox.conf.5 with a custom
command
ci-node-36 | [11/13] Generating src/toolbox with a custom command
https://github.com/containers/toolbox/pull/1158
This mirrors the --preserve-fds option of Podman.
Converting an unsigned 'uint', which is what Podman uses for its
--preserve-fds option, to a string is surprisingly annoying.
strconv.Itoa [1] takes a signed 'int', which would require a cast, and
there's no unsigned counterpart. There's strconv.FormatUint [2] which
takes an unsigned 'uint64', which is better, but would still require a
cast.
So, fmt.Sprint [3] it is, if the cast is to be avoided. It's more
expensive than the other two functions, but there's no need to worry
unless it's proven to be a performance bottle neck.
Some changes by Debarshi Ray.
[1] https://pkg.go.dev/strconv#Itoa
[2] https://pkg.go.dev/strconv#FormatUint
[3] https://pkg.go.dev/fmt#Sprinthttps://github.com/containers/toolbox/issues/1066
Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>
Commit a22d7821cb ensured that a nested pseudo-terminal device is
only created for the process running inside the container, if the Toolbx
binary's standard input and output streams are connected to a terminal.
Therefore, 'echo ...' no longer ends with an unwanted extra carriage
return when terminal devices are absent - there's only a line feed for
the trailing newline. Hence, there's no need to use the -n flag to skip
the trailing newline.
This reverts parts of commit 16b0c5d88f.
https://github.com/containers/toolbox/issues/157
It seems that as new test cases got developed they got appended towards
the end of the file. Now that there are a non-trivial number of test
cases, it's difficult to look at the file and get a gist of all the
scenarios being tested.
It will be better to have some logical grouping -- starting with the
most basic functionality, then moving on to more advanced features,
and then finally the errors.
This is a step towards that.
https://github.com/containers/toolbox/pull/1155
Here's some historical context to understand what's going on.
In the past, before commit a22d7821cb, Podman's standard error
stream was only revealed when --verbose was used.
During that time, the standard error and output streams of the process
running inside the Toolbx container, but not 'podman exec ...' itself,
were merged into the standard output stream read and revealed by the
Toolbx binary.
Then commit a22d7821cb ensured that a nested pseudo-terminal
device is only created for the process running inside the container, if
the Toolbx binary's standard input and output streams are connected to a
terminal. This meant that the standard error stream of the container
process stayed separate from the standard output stream received by the
Toolbx binary, when terminal devices were absent. The errors from
'podman exec ...' itself continued to be separate as before.
However, Toolbx only read and revealed the standard error stream of the
spawned 'podman exec ...' process when --verbose was used. This meant
that all the errors from the container process got lost in the absence
of --verbose. This was an unintended change in behaviour caused by
commit a22d7821cb that got addressed in the subsequent commit
7cba807e45, but with yet another unintended change in behaviour.
Commit 7cba807e45 started reading and revealing the standard
error stream of the spawned 'podman exec ...' process unconditionally.
This caused the errors from both Podman and the container process to be
revealed unconditionally, which is a problem.
Podman is an implementation detail of Toolbx. Therefore, Toolbx users
shouldn't be directly exposed to errors from Podman, unless they are
using --verbose to debug a problem. On the other hand, the container
process is the outcome of a command specified by the user. So, the user
does expect to see what's going on with it.
That's the unintended change in behaviour this commit tries to fix.
Unfortunately, when Toolbx is being used non-interactively (ie., no
terminal devices), the errors from the process running inside the
Toolbx container and the errors from 'podman exec ...' itself are part
of the same standard error stream received by Toolbx. It's impossible
to distinguish between the two without deeper changes.
Hence, this commit only focuses on interactive use (ie., terminals are
present), which is where the visual appearance and presentation of error
messages really matter. Non-interactive use is programmatic use, so the
visuals don't matter so much.
Fallout from 7cba807e45https://github.com/containers/toolbox/pull/1154
The outcome of checking whether the standard input and output of the
current invocation of toolbox are connected to a terminal device is
going to stay constant for the life cycle of the process. So, checking
it repeatedly in a loop when falling back to a different command or
working directory is wasteful.
Secondly, it prevents secondary logic like this from intermingling with
the code that actually assembles the list of arguments. This makes it
easier to get a quick gist of the final command and its structure.
Fallout from a22d7821cb
This needs a directory that's going to be present on the host operating
system across various configurations of all supported distributions,
such as the hosts running the CI, but not inside the Toolbx containers.
It looks like /etc/kernel is present on both Debian and Fedora, but
absent from the fedora-toolbox images. On a Debian 10 server, it's
owned by several packages:
$ dpkg-query --search /etc/kernel
dkms, systemd, grub2-common, initramfs-tools, apt: /etc/kernel
... while on Fedora 36 Workstation:
$ rpm --file --query /etc/kernel
systemd-udev-250.8-1.fc36.x86_64
Currently, there's no way to get assert_line to use the stderr_lines
array [1]. This is worked around by assigning stderr_lines to the
'lines' array.
[1] https://github.com/bats-core/bats-assert/issues/42https://github.com/containers/toolbox/pull/1153
It seems that as new test cases got developed they got appended towards
the end of the file. Now that there are a non-trivial number of test
cases, it's difficult to look at the file and get a gist of all the
scenarios being tested.
It will be better to have some logical grouping -- starting with the
most basic functionality, then moving on to more advanced features,
and then finally the errors.
This a step towards that.
https://github.com/containers/toolbox/pull/1152
Currently, commands invoked using 'toolbox run' have a different
environment than the interactive environment offered by 'toolbox enter'.
This is because 'toolbox run' was invoking the commands using something
like this:
$ bash -c 'exec "$@"' bash [COMMAND]
... whereas, 'toolbox enter' was using something like this:
$ bash -c 'exec "$@"' bash bash --login
In the first case, the helper Bash shell is a non-interactive non-login
shell. This means that it doesn't read any of the usual start-up files,
and, hence, it doesn't pick up anything that's specified in them. It
runs with the default environment variables set up by Podman and the
Toolbx image, plus the environment variables set by Toolbx itself.
In the second case, even though the helper Bash shell is still the same
as the first, it eventually invokes a login shell, which runs the usual
set of start-up files and picks up everything that's specified in them.
Therefore, to ensure parity, 'toolbox run' should always have a login
shell in the call chain inside the Toolbx container.
The easiest option is to always use a helper shell that's a login shell
with 'toolbox run', but not 'toolbox enter' so as to avoid reading the
same start-up files twice, due to two login shells in the call chain.
It will still end up reading the same start-up files twice, if someone
tried to invoke a login shell through 'toolbox run', which is fine.
It's very difficult to be sure that the user is invoking a login shell
through 'toolbox run', and it's not what most users will be doing.
https://github.com/containers/toolbox/issues/1076
For the most part, this fixes a minor cosmetic issue for users, but it
does make the code less misleading to read for those hacking on Toolbx.
Further details below.
Commands are invoked inside a Toolbx from a helper shell invoked by
capsh(1). Unless capsh(1) is built with custom options, the helper
shell is always bash, not /bin/sh:
$ capsh --caps="" -- -c 'echo "$(readlink /proc/$$/exe)"'
/usr/bin/bash
( The possibility of capsh(1) using a different shell, other than Bash,
through a custom build option is ignored for the time being. If there
really are downstream distributors who do that, then this can be
addressed one way or another. )
Secondly, the name assigned to the embedded command string's '$0' should
only be the basename of the helper shell's binary, not the full path, to
match the usual behaviour:
$ bash -c 'exec foo'
bash: line 1: exec: foo: not found
With 'toolbox run' it was:
$ toolbox run foo
/bin/sh: line 1: exec: foo: not found
Error: command foo not found in container fedora-toolbox-36
https://github.com/containers/toolbox/pull/1147
Using 'true' is likely going to be quicker than launching the entire
shell (ie., /bin/sh).
Note that 'toolbox run' already invokes a wrapper shell via capsh(1)
before invoking the user-specified command. So, this was the second
instance of a shell.
https://github.com/containers/toolbox/pull/1145
It was decided in commit 950f510872 that golang.org/x/* would be
used for the IsTerminal() API, not github.com/mattn/go-isatty. However,
github.com/mattn/go-isatty had crept in through commits f49df914f4
and a22d7821cb.
The size savings seem to have been lost, because with Go 1.18.6, the
binary size actually grew from 9410616 bytes to 9410912. However, it
seems better to stick to packages from the golang.org domain, whenever
possible.
https://github.com/containers/toolbox/pull/1144
