89luca89 / distrobox

Use any linux distribution inside your terminal. Enable both backward and forward compatibility with software and freedom to use whatever distribution you’re more comfortable with. Mirror available at: https://gitlab.com/89luca89/distrobox
https://distrobox.it/
GNU General Public License v3.0
9.86k stars 405 forks source link

init: do not fail in "Setting up read-only mounts" if findmnt does not exist #1454

Closed phoppermann closed 3 months ago

phoppermann commented 3 months ago

If findmnt does not exist (or returns an error), "Setting up read-only mounts" in distrobox-init fails with "Error: An error occurred".

Treat missing findmnt the same way as if the file/directory cannot be read.

It seems like some error handling for failing findmnt was already forseen:

flags="$(findmnt --noheadings --output OPTIONS --target "${src}" || :)"

However, the code apparently cannot handle flags=":".

I'm completely unsure if my approach is reasonable or which implications it might have but it allows to at least enter the distrobox.

89luca89 commented 3 months ago

Hi @phoppermann

why does this happen?

findmnt is added on the pkg setup step, it should definitely be there?

phoppermann commented 3 months ago

I don't know. Where is the code which adds findmnt in pkg setup? Then I can take a look what's going on.

89luca89 commented 3 months ago

I don't know. Where is the code which adds findmnt in pkg setup? Then I can take a look what's going on.

it's in the package setup steps, for each package manager there is a list of packages, all of them should contain findmnt

phoppermann commented 3 months ago

https://github.com/89luca89/distrobox/blob/3365ff9ca73f065a3fe36f256d52102226e6b759/distrobox-init#L1375

Apparently, the package does not contain findmnt in the very old version I'm trying to use yet.

However, it doesn't probably doesn't really matter. https://github.com/89luca89/distrobox/pull/1296 should handle this situation as well. Like described in the PR description, this doesn't work (anymore?). So potentially there's also a regression wrt #1289. Solving this would likely fix my issue with missing findmnt as well. EDIT: The issue and PR above is related to a different usage of findmnt.

89luca89 commented 3 months ago

@phoppermann if you give more info about the setup/distro you're going for, I can try to reproduce

phoppermann commented 3 months ago

You can reproduce my problem by changing https://github.com/89luca89/distrobox/blob/24716c1b1802222b6475f3387c89b48948e79f98/distrobox-init#L258 to

flags="$(:)"

which is what would also happen in any error case.

phoppermann commented 3 months ago

It works correctly with

flags=":"

which is probably the intended behavior.

Note that https://github.com/89luca89/distrobox/commit/b79a7059b88f2018d9e07c6265e4aaba767d6ec0 places the || : behind (and not inside of) $().

89luca89 commented 3 months ago

@phoppermann ok got it

can you share with me which guest distro/image you're using? to understand why findmnt is failing

phoppermann commented 3 months ago

It's not failing, it just doesn't exist at all. I'm using a custom image based on an ancient SLES so nothing you could use for testing.

89luca89 commented 3 months ago

ah got it, welp if it doesn't exists we can indeed skip this that's ok

thanks for helping!

phoppermann commented 3 months ago

Ok. There's probably still the problem if findmnt really fails which should be tracked in a separate issue.