-
Notifications
You must be signed in to change notification settings - Fork 220
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
fix: use FQDN with toolbox
prefix
#1086
base: main
Are you sure you want to change the base?
Conversation
Build succeeded. ✔️ unit-test SUCCESS in 6m 54s |
this seems to trigger a minor bug, when exiting the toolbox if I pressed I'll have to dig into that |
Build succeeded. ✔️ unit-test SUCCESS in 6m 57s |
Build failed. ❌ unit-test NODE_FAILURE in 0s |
sorry for the noise the bug I hit seems to be because toolbox exits with return code 130 (SIGTERM) whenever |
why is so convoluted ... it would seems this is equivalent
mm from further experimentation it seems that bash just does this and toolbox without modification does this:
imo this should keep the error code of the last command execute so it should be |
Build failed. ❌ unit-test FAILURE in 7m 15s |
Build failed. ✔️ unit-test SUCCESS in 7m 07s |
Build failed. ✔️ unit-test SUCCESS in 7m 03s |
Build failed. ✔️ unit-test SUCCESS in 7m 10s |
I have continued development of toolbox on my own as I'm not really having this merged on a reasonable time frame. my fork is at: https://github.com/akdev1l/toolbox/tree/akdev I have fixed some long standing issues and added some features (particularly I have enabled a static build and containerized Otherwise I will leave this PR to die - feel free to close. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be good if the commit message had some explanation around exit code 130 and SIGTERM
. Particularly the user facing behaviour.
src/cmd/run.go
Outdated
@@ -477,10 +479,10 @@ func constructExecArgs(container string, | |||
|
|||
execArgs = append(execArgs, []string{ | |||
container, | |||
"capsh", "--caps=", "--", "-c", "exec \"$@\"", "/bin/sh", | |||
"capsh", "--caps=", "--", "-c", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to have anything to do with what the commit message describes. :)
I don't think your changes introduced that. :) A few months ago, Toolbx started propagating the exit code of the command, which triggered this. You wouldn't have encountered this behaviour before that. From the
Hence, 130 (= 128 + 2) as the exit code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, @akdev1l ; and my apologies for the delay.
I am curious. I have never experienced delays when using |
The way things stand today, we are unlikely to build statically or disable CGO. See: #832 Of course, this may change if our realities change or if new information comes to light. |
@debarshiray hi! thanks for your comments - I'll clean this up and just deal with the fqdn change you gave me some historical background on toolbx and I appreciate that.
this happened to me because I build/distribute my own toolbx images (https://github.com/akdev1l/toolbox-images) - there is a requirement that isn't specified in the documentation which is having |
nod @HarryMichal explained that to me later in real life. My apologies, I forgot to mention that here. |
@debarshiray minimal cleaned this up, this should be the minimal change required for a proper FQDN - looks like it passes the tests I still think we should change the prefix from "toolbox" to the name of the toolbox, bash prompts seem to show the first component of the FQDN (with this change there won't be any user visible prompt changes as we have hardcoded it the first component to
so this would solve the issue of distinguishing the containers very elegantly imo (no changes required on the images, no big changes required on toolbox, no custom scripts nor custom prompts for bash or other shells) |
toolbox
as constant for FQDNtoolbox
prefix
Build succeeded. ✔️ unit-test SUCCESS in 8m 14s |
Signed-off-by: Alex Diaz <[email protected]>
Build succeeded. ✔️ unit-test SUCCESS in 8m 14s |
@debarshiray Does this PR need anything else before merging? |
Hey @akdev1l, sorry for the delay on this PR. I am reviewing everything to merge it as soon as possible. I just wanted to ask a quick question. At the beginnning of the issue, in the initial explanation, you say that the expected container hostname is the following |
As someone who uses multiple toolboxes (one for regular development work, and a "grab-bag" one for other stuff), I would prefer the container name. (But it's not a big deal, honestly, since I can always change the prompt from within the toolbox itself.) |
I also wanted to reproduce those timeouts you were talking about. Could you please let me know which commands led you to those timeouts so that I can reproduce them on my machine? |
Personally I encountered them mostly when using Jetbrains IDEs, e.g. IntelliJ, CLion, from within toolbox on a Fedora Kinoite (KDE plasma) system, but it should be possible to reproduce with other GUI apps on a KDE system because the problems seem to be with kwin trying to do some DNS resolution in relation to putting the host name in the title bar of the windows (" |
+1 for using the container name for the hostname. |
I just realized something: With the Foot terminal (and others), I can emit OSC 7 on directory change to have new terminals spawn in that directory. However, if you look at the linked code snippet:
You can see that this depends on the correct hostname (so that it doesn't erroneously catch directory changes over SSH connections, for example). So, the toolbox not having the host's hostname breaks at least that bit of functionality. |
@juhp Distrobox has now switched to NOT set the hostname to container name because messing with the hostname caused too many problems. See |
Issue: #969
Setting the hostname to
toolbox
causes timeouts whenever anything triesto resolve the name of the machine - for example
sudo
does this.This change makes it so the FQDN is set to
${container_name}.${hostname}
as recommended in the linked issue.After this change commands can properly resolve the local FQDN.
I removed the symlink to
/run/host/etc/hosts
because podman already copies that information in and then we can use--add-host
to add a mapping to localhost for the container - this way callingping $(hostname)
does what is expected.Pending PRs:
container-name.hostname
convention otherwise it seems confusingNone of these PRs address my issue with delays due to unresolvable hostnames. So this one tries to do that.
Sample Output