Deprecation message when using 0.0.96 and older Toolbx container + tests #1697

DaliborKr · 2025-08-06T08:02:33Z

Added deprecation message when running a container that was created with Toolbx 0.0.96 and older, as described in #1684

softwarefactory-project-zuul · 2025-08-06T09:27:29Z

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/9cfd501024474d8298afae4c2a409a19

debarshiray

Thanks for working on this, @DaliborKr ! I didn't get a chance to try out the code changes and the test, so I only have some superficial comments.

Could you please add a link to the issue #1684 in the Git commit message, like we do in other commits? It makes it easier to follow the context of the commits in future. Otherwise, one has to open the commit in a web browser and then find the links.

There are some minor trailing whitespace and missing newline problems. The trailing whitespaces can cause confusion in future, because they can get deleted or added in future commits and cause false extra lines to show up in the diff. Git can help you detect them, if you use:

$ git config --global diff.wsErrorHighlight all

... or put this in your ~/.gitconfig:

[diff]
       wsErrorHighlight = all

test/system/libs/helpers.bash

test/system/104-run.bats

softwarefactory-project-zuul · 2025-08-06T15:06:42Z

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/ca1cf102a05545e6b0afcd15320ab3af

softwarefactory-project-zuul · 2025-09-07T13:23:56Z

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/5820a80495064895afcb3aef961e5d04

debarshiray

Thanks for working on this, @DaliborKr ! The changes mostly look good to me.

debarshiray · 2025-09-08T23:44:14Z

test/system/104-run.bats

@@ -863,3 +863,36 @@ teardown() {
  assert_line --index 1 "Recreate it with Toolbx version 0.0.17 or newer."
  assert [ ${#stderr_lines[@]} -eq 2 ]
 }
+
+@test "run: Ensure that label 'com.github.containers.toolbox=true' is present in toolbx container" {


Some relatively minor points:

We should split this new test into a separate commit, because it's unrelated to the deprecation of old containers that use org.freedesktop.Flatpak.SessionHelper, as described in #1684

This is a test for the create command because it's checking if it added the com.github.containers.toolbox label to the container it created. So, it should go in test/system/101-create.bats.

The new thing that this test adds is the check for the label in the containers created by create. So, it can be merged with the existing tests for create by adding the podman inspect to them. We already use podman ps --all to check if the container was actually created, and this can be the next assertion.

I took the liberty to address these in #1716 because they were relatively minor issues.

debarshiray · 2025-09-08T23:50:02Z

test/system/104-run.bats

+
+  create_container "$container_name"
+
+  run podman inspect --format '{{ index .Config.Labels "com.github.containers.toolbox" }}' "$container_name"


Cool! I didn't know that we can use index with a JSON dictionary. It would have made my life easier in the past. :)

debarshiray · 2025-09-08T23:51:38Z

test/system/104-run.bats

+  run podman inspect --format '{{ index .Config.Labels "com.github.containers.toolbox" }}' "$container_name"
+
+  assert_success
+  assert_output --regexp "true"


We shouldn't use the --regexp option here, because we aren't using any regular expressions, and there's no reason to expect anything other than true in the output. So, just:

assert_output "true"

Done in #1716

debarshiray · 2025-09-09T13:01:08Z

test/system/libs/helpers.bash

+function create_container_flatpak_dbus() (
+  local container_name
+
+  container_name="$1"


Nitpick: these two lines can be combined.

I know there are some places in the code base where the lines are split, but it is an anti-pattern caused by a misunderstanding. It's because ShellCheck complains about cases where command substitution is used.

debarshiray · 2025-09-09T13:01:50Z

test/system/libs/helpers.bash

+  pull_default_image
+  distro="$(get_system_id)"
+  version="$(get_system_version)"
+  image_full="${IMAGES[$distro]}:$version"


There's already a get_default_image function that does this. :)

test/system/libs/helpers.bash

debarshiray · 2025-09-09T13:09:29Z

test/system/libs/helpers.bash

+        return 1
+    fi
+
+    echo "$output" | grep -o "/.*/monitor"


Interesting. I didn't know about the -o option in grep(1). Looks very useful. :)

I see that the output from the D-Bus method is:

({'path': <'/run/user/1000/.flatpak-helper/monitor'>, 'pkcs11-socket': <'/run/user/1000/.flatpak-helper/pkcs11-flatpak-39343'>},)

The problem is that if we parse the path by looking for "/.*/monitor", then we are assuming that the path will always end with /monitor. This defeats the purpose of querying the path from the D-Bus service, instead of assuming it and hard coding it.

Interestingly, the last time the path changed was in June 2018. Before that it used to be $XDG_RUNTIME_DIR/flatpak-monitor, and that value wouldn't fit "/.*/monitor".

I don't know what would be the best or easiest way to parse the path from our test suite. It's in the GVariant format and there are various ways to encode and decode it. The Go implementation of Toolbx uses github.com/godbus/dbus/v5 to parse it in CallFlatpakSessionHelper().

Until we figure out a correct way to parse it, let's just assume that it will be $XDG_RUNTIME_DIR/.flatpak-helper/monitor and hard code it. It used to be hard coded in the POSIX shell implementation and it hasn't changed in the last seven years. It's better to hard code, than have some extra code that misleadingly looks like it's doing it correctly, when it's not. :)

debarshiray · 2025-09-09T13:11:56Z

src/cmd/run.go

@@ -529,6 +529,9 @@ func callFlatpakSessionHelper(container podman.Container) error {
 		return nil
 	}

+	fmt.Fprintf(os.Stderr, "Warning: container %s uses deprecated features\n", container.Name())
+	fmt.Fprintf(os.Stderr, "Consider recreating it with Toolbox version 0.0.97 or newer.\n")


We should also update the recommended version a few lines above from 0.0.17 to 0.0.97, because containers created with 0.0.17 to 0.0.96 are now deprecated.

softwarefactory-project-zuul · 2025-09-09T14:37:53Z

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/7a86e114777e44d8a5128f714bb83359

debarshiray · 2025-09-10T00:08:28Z

test/system/libs/helpers.bash

+    )
+
+    # Print the full command
+    # printf "%q " "${podman_cmd[@]}" >> /tmp/toolbox_podman_command.log


We can use bats --trace ... for this. It will show all the commands that get executed.

softwarefactory-project-zuul · 2025-09-10T01:50:48Z

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/ea1bb33f9fb24fafa95dcc4c9648a1ef

debarshiray · 2025-09-10T13:10:42Z

test/system/libs/helpers.bash

+          --dns none \
+          --env TOOLBOX_PATH="$TOOLBX" \
+          --name "$container_name" \
+          --network host \


It might be good to add the labels, because you have been working on checking the labels, and it will let us add another test to test/system/102-list.bats to ensure that these deprecated containers are listed.

debarshiray · 2025-09-10T13:18:18Z

test/system/libs/helpers.bash

+          "$default_image" \
+          toolbox init-container \
+                  --home "$HOME" \
+                  --shell "$SHELL" \


Let's add the --monitor-host option to init-container here.

We deprecated --monitor-host and stopped using it from Toolbx 0.0.99.5. It means that containers that were created by Toolbx < 0.0.97 to use org.freedesktop.Flatpak.SessionHelper still have the option, and init-container should continue to handle it. It is hard or rare to test deprecated code paths, so using it in this test is a good opportunity to test it.

debarshiray · 2025-09-10T13:27:43Z

test/system/libs/helpers.bash

+    # Print the full command
+    # printf "%q " "${podman_cmd[@]}" >> /tmp/toolbox_podman_command.log
+
+    "${podman_cmd[@]}" || fail "Podman couldn't create container '$container_name'"


Nitpick: we can use echo and return 1, like you had used when resolving the home directory and parsing the path used by org.freedesktop.Flatpak.SessionHelper.

I know fail is used in some other parts of the code base. However, over time, it seems to me that it doesn't have that many benefits over printing a message to the standard error stream and returning with a non zero value. The fail function isn't part of core Bats and comes from bats-support, which is sometimes not packaged by distributions, and we have to carry it as a Git submodule in Toolbx. If we stop using it, then we can drop this dependency.

debarshiray · 2025-09-10T13:28:45Z

test/system/104-run.bats

+  assert [ ${#lines[@]} -eq 0 ]
+  lines=("${stderr_lines[@]}")
+  assert_line --index 0 "Warning: container $container uses deprecated features"
+  assert_line --index 1 "Consider recreating it with Toolbox version 0.0.97 or newer."


It should be Toolbx without the o. :)

Starting from Toolbx 0.0.97, released in November 2020, Toolbx containers no longer need the org.freedesktop.Flatpak.SessionHelper D-Bus interface. This change was made to enable running toolbox(1) as root [1]. It is going to be five years in a few months, since this change was made, and it's becoming harder and harder to practically test backwards compatibility with these containers. All the host operating systems where Toolbx is regularly tested offer newer versions: * Arch Linux and Fedora offer the latest upstream releases * RHEL 8.10 offers at least 0.0.99.4 * Ubuntu offers at least 0.0.99.2 [2] Therefore, this is a good time to mark these older containers as deprecated, as has been done in the past in similar cases [3]. A deprecation notice is the precursor to actually dropping support for these old containers in the future. [1] Commit 71b5c8c containers@71b5c8c0a235249b containers#591 containers#267 [2] https://packages.ubuntu.com/jammy/podman-toolbox [3] Commit 9dc5281 containers@9dc52814301c5a3f containers#336 containers#1684

softwarefactory-project-zuul · 2025-09-10T15:16:28Z

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/a5763c5898c544d9bd3bb3d508f3c47a

debarshiray · 2025-09-10T15:35:35Z

One thing that struck me while playing with this pull request, is that the deprecated container created by the test suite doesn't suffer from containers/crun#1841 even though we are not using podman create --mount type=devpts,destination=/dev/pts.

I tried to compile Toolbx 0.0.96 on Fedora 42 and the container it created did hit containers/crun#1841 and I failed to enter it. I wonder what is it about the deprecated container created by the test suite that it doesn't have this problem.

debarshiray · 2025-09-10T15:59:19Z

Anyway, merged! Thanks for working on this.

DaliborKr requested a review from debarshiray as a code owner August 6, 2025 08:02

debarshiray reviewed Aug 6, 2025

View reviewed changes

test/system/libs/helpers.bash Outdated Show resolved Hide resolved

test/system/104-run.bats Outdated Show resolved Hide resolved

test/system/104-run.bats Outdated Show resolved Hide resolved

test/system/104-run.bats Outdated Show resolved Hide resolved

DaliborKr force-pushed the depr_flatpak_dbus_service branch from 494ab56 to 419bbba Compare August 6, 2025 13:46

debarshiray force-pushed the depr_flatpak_dbus_service branch from 419bbba to 03ad7d2 Compare September 7, 2025 11:57

debarshiray reviewed Sep 8, 2025

View reviewed changes

debarshiray reviewed Sep 9, 2025

View reviewed changes

debarshiray force-pushed the depr_flatpak_dbus_service branch from 03ad7d2 to 7f5d314 Compare September 9, 2025 13:12

debarshiray force-pushed the depr_flatpak_dbus_service branch from 7f5d314 to bc7085d Compare September 9, 2025 23:23

debarshiray reviewed Sep 10, 2025

View reviewed changes

debarshiray force-pushed the depr_flatpak_dbus_service branch from bc7085d to 1b380a4 Compare September 10, 2025 00:23

debarshiray reviewed Sep 10, 2025

View reviewed changes

debarshiray force-pushed the depr_flatpak_dbus_service branch from 1b380a4 to af1216b Compare September 10, 2025 13:56

debarshiray merged commit af1216b into containers:main Sep 10, 2025
3 checks passed

debarshiray mentioned this pull request Sep 10, 2025

Deprecate containers that need the org.freedesktop.Flatpak D-Bus service #1684

Closed


		create_container "$container_name"

		run podman inspect --format '{{ index .Config.Labels "com.github.containers.toolbox" }}' "$container_name"

Deprecation message when using 0.0.96 and older Toolbx container + tests #1697

Deprecation message when using 0.0.96 and older Toolbx container + tests #1697

Uh oh!

Conversation

DaliborKr commented Aug 6, 2025

Uh oh!

softwarefactory-project-zuul bot commented Aug 6, 2025

Uh oh!

debarshiray left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

softwarefactory-project-zuul bot commented Aug 6, 2025

Uh oh!

softwarefactory-project-zuul bot commented Sep 7, 2025

Uh oh!

debarshiray left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softwarefactory-project-zuul bot commented Sep 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softwarefactory-project-zuul bot commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softwarefactory-project-zuul bot commented Sep 10, 2025

Uh oh!

Uh oh!

debarshiray commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debarshiray commented Sep 10, 2025

Uh oh!

Uh oh!

debarshiray commented Sep 10, 2025 •

edited

Loading