Add --mksquashfs-opt #1188
Add --mksquashfs-opt #1188
Conversation
|
Wow, even with tests. 👍 |
TheAssassin
left a comment
TheAssassin
left a comment
There was a problem hiding this comment.
I got some minor change requests. Otherwise this looks good.
Using a parameter like this which you can specify more than once seemed most promising and easier to implement while thinking about it, and your PR shows this is the case. A small and easy to understand change. Good job!
| hash2=$(sha256sum appimagetool.AppImage.2 | awk '{print $1}') | ||
| hash3=$(sha256sum appimagetool.AppImage.3 | awk '{print $1}') | ||
| if [ "$hash1" != "$hash2" ]; then | ||
| echo "Hashes of regular and mem-restricted AppImages differ" |
There was a problem hiding this comment.
I don't quite like the method used in this check, but I guess you can't check -mem otherwise, so this is okay. Should catch most future issues.
It's good you call those checks, not tests.
src/appimagetool.c
Outdated
| char* args[32]; | ||
| guint sqfs_opts_len = sqfs_opts ? g_strv_length(sqfs_opts) : 0; | ||
|
|
||
| char* args[32 + sqfs_opts_len]; |
There was a problem hiding this comment.
The 32 here always seemed arbitrary. Let's put a TODO above it, or fix this properly.
There was a problem hiding this comment.
I counted these manually and it looks like the max is len(sqfs_opts) + 22. I thought about adding a G_STATIC_ASSERT() But I guess this is not "static enough" for a compile time check.
src/appimagetool.c
Outdated
| args[i++] = "-mkfs-time"; | ||
| args[i++] = "0"; | ||
|
|
||
| for (guint sqfs_opts_idx = 0; sqfs_opts_idx < sqfs_opts_len; sqfs_opts_idx++) { |
There was a problem hiding this comment.
Sure, the compiler will optimize this anyway, but it'd be good style to use ++sqfs_opts_idx.
|
Thanks for reviewing @TheAssassin |
|
I have a patch for the bug I mentioned in the description (appimagetool does not die if mksquashfs fails): https://github.com/lalten/AppImageKit/compare/add-mksquashfs-opts-arg..die-on-mksquashfs-failure @TheAssassin let me know if you want any changes or I should squash so we can merge this one :) |
|
I'll just squash on GitHub should the build pass. |
|
Thanks @lalten. |
3 participants
This PR adds a new
--mksquashfs-optargument to appimagekit. It can be used to pass through arbitrary options tomksquashfs. For example to limit mksquashfs memory consumption to 100MB you can invoke apppimagekit likeNote that specifying invalid arguments to mksquashfs will print error messages, but appimagetool will still claim success and exit with return code 0. I believe this is the existing behaviour. This should probably be fixed as well, but in a separate issue/PR.
Closes #1186