Conversation
In Ubuntu 20.04, this worked flawlessly. But only by chance. We use libraries from the host system, together with the binary inside the mounted appimage. in Ubintu 22.04, this crashes: /tmp/.mount_inkscaYGPL8G/usr/bin/inkscape --version /tmp/.mount_inkscaYGPL8G/usr/bin/inkscape: error while loading shared libraries: libboost_filesystem.so.1.71.0: cannot open shared object file: No such file or directory The fix is to not call the binary directly, but the AppRun scrit, which nicely prepares the environment. AppRun also prints a message for the user to stdout, (instead of stderr). We now need to filter this, when parsing output from e.g. --version: /tmp/.mount_inkscaYGPL8G/AppRun --version 2>/dev/null You should not use AppImage in production, but you can speedup the AppImage by following this guide: https://inkscape.org/learn/appimage/ Inkscape 1.3.2 (091e20e, 2023-11-25)
dirname() should be fine to. It is more readable. Thanks! (not sure if introducing a new apprun_path variable contributes to readability though ...) But I don't mind. Co-authored-by: TheAssassin <theassassin@assassinate-you.net>
|
I'll open a separate PR to fix the issues in the linux-checkinstall CI. |
|
Fix for the CI problems available in #722. |
|
Please also report the stdout pollution as an upstream issue with Inkscape. We are probably not the only one affected by that. ( Side topics, not really relevant to this PR: What about the other combinations and directions - does calling Visicut from the AppImage Inkscape work correctly, even with all the AppImage environment variables? What if VisiCut is also an AppImage? ) |
|
If someone opens an issue upstream, please ask them to just |
Yes, I believe most real-life combinations work. I have especially tested the case where both, Inkscape and VisiCut are AppImages. |
This is broken currently and will be working once this has been merged. |
| INKSCAPEBIN = apprun_path | ||
| for parent in Path(INKSCAPEBIN).parents: | ||
| apprun = parent / "AppRun" | ||
| if apprun.is_file() and os.access(apprun, os.X_OK): |
There was a problem hiding this comment.
I like this os.access check, it's very compact. pathlib should have a shortcut for that.
There was a problem hiding this comment.
Maybe only checking is_file() alone is even more compact? An inkscape AppImage that has an 'AppRun' file which has no execute bits, or is mounted without exec, or suffering from a security policy, or whatever ... is out of scope here anyway.
There was a problem hiding this comment.
Just failing silently is not the best idea. Your algorithm can still cross mountpoints (that's what the is_mount check was there for before) and I can imagine a bunch of false positive detections of AppRuns in that case. For instance, in some occasions, AppDirs can be integrated in each other (lazy approach but it works, I use that myself occasionally). In such a case, the execute bit on some AppRun in the chain might not be set. Also, AppImages can be extracted and copied to devices that do not have the executable bit set, etc.
The check is fine and it's good to have it. In fact, security wise, we should add the mountpoint check and abort once it's crossed. To do so, this should be appended to the for's body:
if parent.is_mount():
breakSecurity (i.e., preventing critical issues) and safety (i.e., keeping users from experiencing weird behavior) always add a bit of verbosity to the code, but in the end, it's always worth it. We'd have some good reasons to throw exceptions here even. But that'd bloat this PR. I think in its current form, it's a reasonably solid implementation and I don't see any attack vector (without thinking about it for more than 5 minutes).
There was a problem hiding this comment.
I found an INKSCAPE_COMMAND environment variable that seems to point to the AppRun file. Can you check if we can use this instead? First guess:
INKSCAPEBIN = os.environ.get("INKSCAPE_COMMAND") or which("inkscape", [INKSCAPEDIR])
This would avoid the potential problems.
There was a problem hiding this comment.
@jnweiger Could you please change to use INKSCAPE_COMMAND (and check if it works), so that we can merge?
There was a problem hiding this comment.
I no longer remember this issue. Sorry. What should we do?
First check if INKSCAPE_COMMAND exists, if not try INKSCAPE_BIN, if not try to find AppRun with one of the loops discussed here?
Or just use INKSCAPE_COMMAND and fail if it fails?
I cannot comment on issues caused by "AppDirs integrated in each other" -- if an is_mount() check helps, then please add that where appropriate.
I am happy to test drive a PR that incorporates all these wishes and concerns that we collected here.
But writing that PR is beyond me. Sorry.
There was a problem hiding this comment.
My suggestion would be to:
- use INKSCAPE_COMMAND if available (should definitely work for Inkscape >= 1.3.x, even in AppImage)
- else determine INKSCAPEBIN from PATH as we did before (maybe needed for very very old Inkscape versions)
- do not implement anything special for AppImage (because they set INKSCAPE_COMMAND).
So basically replace all code that tries to determine the inkscape path with:
INKSCAPEBIN = os.environ.get("INKSCAPE_COMMAND") or which("inkscape", [INKSCAPEDIR])
If that doesn't work we can still add more complexity.
| INKSCAPEBIN = apprun_path | ||
| for parent in Path(INKSCAPEBIN).parents: | ||
| apprun = parent / "AppRun" | ||
| if apprun.is_file() and os.access(apprun, os.X_OK): |
There was a problem hiding this comment.
I found an INKSCAPE_COMMAND environment variable that seems to point to the AppRun file. Can you check if we can use this instead? First guess:
INKSCAPEBIN = os.environ.get("INKSCAPE_COMMAND") or which("inkscape", [INKSCAPEDIR])
This would avoid the potential problems.
|
What does "found" mean? Is that an officially supported environment variable provided by the Inkscape code? Is it an AppImage-specific feature? |
|
For Inkscape 1.3.x and 1.4.x (current / upcoming release), the AppImage sets For Inkscape 1.5.x (development branch) the AppImage is currently broken and fails to launch Python, so it is not possible to test. https://gitlab.com/inkscape/inkscape/-/issues/4851 Inkscape's Once we drop support for AppImage-Inkscape < 1.3 and normal Inkscape < 1.0 (used e.g. on Ubuntu 20.04) we could throw out half of our extension code and instead use |
The SyntaxWarning for invalid escape sequences (such as '\.') started as a DeprecationWarning in Python 3.6 and was made into a SyntaxWarning in Python 3.12.
the button to install the extension is already disabled if the extension isn't installable. hiding the entire menu creates unnecessary confusion.
"Add to" creates unnecessary confusion when used accidentally. nobody ever intentionally used "Add to" anyway, right?
This reverts commit bf794a7. didn't make the splash look better.
|
Hi @tchilov, I see lots of new changes by you in this PR that seem helpful but are not related to the topic of this pull request. Feel free to create separate pull requests for these changes. Let's work together to get these into the mainline of VisiCut, so that all labs can profit from the development, and that your lab can profit from updates in mainline VisiCut. I'm happy to assist, in real life or here in the digital world. |
|
@mgmax I didn't expect pushing to our master affecting this PR. Had I wanted to bring features upstream, I would have created feature branches and PRs. |
|
I am temporarily closing this PR because it contains many commits from your "master" branch that have nothing to do with the PR itself. Every time I get new notifications. Feel free to reopen the PR against a separate branch. Would be cool to get the actual bugfix into VisiCut. |
4 participants
In Ubuntu 20.04, this worked flawlessly. But only by chance. We use libraries from the host system, together with the binary inside the mounted appimage. in Ubintu 22.04, this crashes:
/tmp/.mount_inkscaYGPL8G/usr/bin/inkscape --version
/tmp/.mount_inkscaYGPL8G/usr/bin/inkscape: error while loading shared libraries: libboost_filesystem.so.1.71.0: cannot open shared object file: No such file or directory
The fix is to not call the binary directly, but the AppRun script, which nicely prepares the environment. But then, AppRun also prints a message for the user to stdout, (instead of stderr). We now need to filter this, when parsing output from e.g. --version:
/tmp/.mount_inkscaYGPL8G/AppRun --version 2>/dev/null
You should not use AppImage in production, but you can speedup the AppImage by following this guide: https://inkscape.org/learn/appimage/
Inkscape 1.3.2 (091e20e, 2023-11-25)