Skip to content

Conversation

@liushuyu
Copy link
Member

@liushuyu liushuyu commented Jan 27, 2018

Yet another so-called up-port merge before it's too late


This change is Reviewable

@liushuyu
Copy link
Member Author

Basic tests have manually done on a device running Arch Linux (amd64)
With the following related packages installed:

Package Version
gcc 7.2.1 20180116
glibc 2.26
wine 2.21-staging
Qt 5.10.0
fltk 1.3.4

@liushuyu
Copy link
Member Author

Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

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

Using system mksquashfs may cause an error on some system. That's why we use bundled one. @tresf Right?

@liushuyu
Copy link
Member Author

@PhysSong Sorry, I haven't been in the development for a while now, I didn't get catch up with your progress and your discussions.

@liushuyu
Copy link
Member Author

The Travis CI complaint about cannot find mksquashfs which should be used to work (otherwise you wouldn't add this segment, right?)

@PhysSong
Copy link
Member

@liushuyu There was a discussion about AppImage script error on Discord #devtalk. Fixing the issue looks tricky for now.

@PhysSong
Copy link
Member

Anyway, the only conflicting file is en.ts, and it seems to be covered in this merge(by @liushuyu you). Good job. 👍

@liushuyu
Copy link
Member Author

If I add ARCH=x86_64, then another error occurs: appimagetool uses system mksquashfs instead of bundled one.

@PhysSong Oh, I see, AppImage builder just plainly refuse to use the system mksquashfs am I correct about this?

@PhysSong
Copy link
Member

Yes. For some reason the builder doesn't work with system mksquashfs after some upstream changes.

@tresf
Copy link
Member

tresf commented Jan 27, 2018

A viable AppImage for now should be able to use this:

diff --git a/cmake/linux/package_linux.sh.in b/cmake/linux/package_linux.sh.in
index 7f500e6..ea35383 100644
--- a/cmake/linux/package_linux.sh.in
+++ b/cmake/linux/package_linux.sh.in
@@ -73,8 +73,9 @@ elif ! find "$LINUXDEPLOYQT" -mtime -$DAYSOLD 2>/dev/null|grep -q ".
        "$LINUXDEPLOYQT" --appimage-extract > /dev/null 2>&1
        mv "squashfs-root/usr/bin/appimagetool" "$APPIMAGETOOL"
        success "Extracted $APPIMAGETOOL"
-       mv "squashfs-root/usr/bin/mksquashfs" "$USERBIN/mksquashfs"
-       success "Extracted $USERBIN/mksquashfs"
+       mkdir -p "$USERBIN/../lib"
+       mv "squashfs-root/usr/lib/appimagekit/" "$USERBIN/../lib/"
+       success "Extracted $USERBIN/../lib/mksquashfs"
        rm -rf "squashfs-root/"
 
 else

The fix should be applied to stable-1.2, but a better fix is to just take the custom ~/bin logic out. As @PhysSong indicates, mksquashfs will only work with the AppImage version. More details about this here: AppImage/AppImageKit#592

@liushuyu
Copy link
Member Author

@tresf @PhysSong Still broken, is there a way to see the log file in the VM?

@PhysSong
Copy link
Member

Adding this two lines seems to fix the issue:

mv "squashfs-root/usr/bin/desktop-file-validate" "$USERBIN/desktop-file-validate"
success "Extracted $USERBIN/desktop-file-validate"

See https://travis-ci.org/PhysSong/lmms/jobs/327108566#L5220 and AppImage/AppImageKit@8a471fc

@PhysSong
Copy link
Member

@liushuyu The new en.ts contains some strings from src/3rdparty/qt5-x11embed/3rdparty/ECM, which should be excluded according to your script. Any ideas?

@liushuyu
Copy link
Member Author

Adding this two lines seems to fix the issue:

@PhysSong Included in 87f4bfb

@liushuyu The new en.ts contains some strings from src/3rdparty/qt5-x11embed/3rdparty/ECM, which should be excluded according to your script. Any ideas?

I forgot to run the checker script when I was merging this, sorry. It's fixed in 2dab6dc

@liushuyu
Copy link
Member Author

Packaged Version for this PR:

OS URL
GNU/Linux (amd64, kernel 3.4+) w/ GLibc https://transfer.sh/16Pbu/lmms-1.2.0-rc5.6477-linux-x86_64.AppImage
Windows (R) 32-bit https://transfer.sh/NzhuJ/lmms-1.2.0-rc5.6477-win32.exe
Windows (R) 64-bit https://transfer.sh/zyRlI/lmms-1.2.0-rc5.6477-win64.exe
macOS (amd64) 10.12+ (Sierra) https://transfer.sh/oJRoG/lmms-1.2.0-rc5.6477-mac10.12.dmg

@liushuyu
Copy link
Member Author

So is this good to merge though?

@PhysSong
Copy link
Member

PhysSong commented Jan 28, 2018

Before merging, I suggest:

  1. Squash commit 2dab6dc(i18n fixup) into the merge commit
  2. Squash two package_linux.sh.in commit into a single commit(and optionally edit commit message)

Any other things look good for me. 👍

@liushuyu
Copy link
Member Author

  1. Squash commit 2dab6dc(i18n fixup) into the merge commit

Sorry this will screw up git rebase mechanism, so I merged two i18n related commits together and two package_linux.sh commits together instead

@tresf
Copy link
Member

tresf commented Jan 28, 2018

@liushuyu, spaces vs. tabs on package_linux.sh.in.

-        mv "squashfs-root/usr/bin/desktop-file-validate" "$USERBIN/desktop-file-validate"
-        success "Extracted $USERBIN/desktop-file-validate"
+	mv "squashfs-root/usr/bin/desktop-file-validate" "$USERBIN/desktop-file-validate"
+	success "Extracted $USERBIN/desktop-file-validate"

@PhysSong
Copy link
Member

PhysSong commented Feb 3, 2018

@liushuyu Please squash two package_linux.sh.in commit into a single commit again. Then I'd like to say go ahead. 🚜

@PhysSong
Copy link
Member

@liushuyu I think doing a new merge may be better since some commits have been added into stable-1.2 and AppImage issue will be fixed when/if #4186 is merged.

@PhysSong
Copy link
Member

PhysSong commented Mar 7, 2018

Just pushed 0a5d056 instead. Anyway, thanks to @liushuyu for reminding us.
@liushuyu Could you please sync i18n strings with Transifex(which have not been done from current master) again?

@PhysSong PhysSong closed this Mar 7, 2018
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.

7 participants