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

Find usermountN and set FUSERMOUNT_PROG if not set #82

Merged
merged 4 commits into from
Nov 24, 2024

Conversation

probonopd
Copy link
Member

@probonopd probonopd commented Nov 24, 2024

This approach avoids tricky patching of upstream code and exports FUSERMOUNT_PROG instead.

Closes #15, #16, #31, #32, #35, #36, #37

@probonopd probonopd changed the title Find usermountN using execve and set FUSERMOUNT_PROG if not set Find usermountN and set FUSERMOUNT_PROG if not set Nov 24, 2024
@AppImage AppImage deleted a comment from github-actions bot Nov 24, 2024
probonopd added a commit to probonopd/PrusaSlicer that referenced this pull request Nov 24, 2024
Copy link

Build for testing:
artifacts
Use at your own risk.

@AppImage AppImage deleted a comment from github-actions bot Nov 24, 2024
@probonopd probonopd enabled auto-merge (squash) November 24, 2024 11:15
@probonopd
Copy link
Member Author

probonopd commented Nov 24, 2024

Not quite just yet... This happens when I move fusermount and fusermount3 to fusermount98 and fusermount99, respectively:

image

Probably because trying to execvp it without arguments fails. Hence adding --version.

@AppImage AppImage deleted a comment from github-actions bot Nov 24, 2024
Copy link

github-actions bot commented Nov 24, 2024

Build for testing:
artifacts
Use at your own risk.

artifacts.zip

@AppImage AppImage deleted a comment from github-actions bot Nov 24, 2024
Copy link
Member

@TheAssassin TheAssassin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't had a chance to check the find fusermount function fully yet. But the approach itself looks solid and the rest of the code, too. Please see my comments.

.github/workflows/build.yaml Outdated Show resolved Hide resolved
scripts/common/install-dependencies.sh Outdated Show resolved Hide resolved
src/runtime/runtime.c Outdated Show resolved Hide resolved
@probonopd
Copy link
Member Author

So far it works on NixOS by default, which is an improvement over what we had without this PR.
It finds and uses /run/wrappers/bin/fusermount3.

However, as soon as one starts experimenting with

sudo mv /run/wrappers/bin/fusermount /run/wrappers/bin/fusermount98
sudo mv /run/wrappers/bin/fusermount3 /run/wrappers/bin/fusermount99

it does not use these but uses /run/wrappers/bin/fusermount3, and as a result we get fusermount3: mount failed: Operation not permitted. And exporting FUSERMOUNT_PROG manually seemingly gets ignored.

Copy link

github-actions bot commented Nov 24, 2024

Build for testing:
artifacts x86_64
artifacts i686
artifacts armhf
artifacts aarch64
Use at your own risk.

artifacts x86_64.zip

@probonopd
Copy link
Member Author

Works on NixOS. 👍
Set FUSERMOUNT_PROG_DEBUG=1 for additional debugging output.

@AppImage AppImage deleted a comment from github-actions bot Nov 24, 2024
@AppImage AppImage deleted a comment from github-actions bot Nov 24, 2024
@TheAssassin
Copy link
Member

I cleaned up the branch and fixed all remaining issues. I'm not a fan of that search function (too complex and using outdated API), but then again, it works fine for now and it doesn't leak.

We still need the patches to FUSE to support the env var. I changed their behavior to demand this input value (which is set by the runtime usually but can be overwritten by the user if needed, albeit we do not want to document any of this). There's also a new debug feature for our FUSE patches. It is what it is. This is the best possible version.

@probonopd please open an issue around upstreaming our patch, possibly with some additional modifications.

@probonopd
Copy link
Member Author

Since upstream does not accept feature requests anymore but PRs, we'd need to turn

https://github.com/AppImage/type2-runtime/blob/e32d17652038e95210abc8b0334ba52da31434a4/patches/libfuse/mount.c.diff

into something suitable for a PR, and I think for that we'd need to make that environment variable optional rather than mandatory.

Copy link
Member Author

@probonopd probonopd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Pull request authors can't approve their own pull request", but I am happy with this and am advocating for it to be merged. Thanks @TheAssassin

@TheAssassin TheAssassin merged commit 59c34ff into main Nov 24, 2024
18 checks passed
@TheAssassin
Copy link
Member

we'd need to turn

https://github.com/AppImage/type2-runtime/blob/e32d17652038e95210abc8b0334ba52da31434a4/patches/libfuse/mount.c.diff

into something suitable for a PR

Well...

@probonopd please open an issue around upstreaming our patch, possibly with some additional modifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fuse: failed to exec fusermount: Permission denied
2 participants