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

[RFC] Appimage #6638

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@AdnoC
Contributor

AdnoC commented May 2, 2017

Handles #6083

Command to build the AppImage executable is make appimage.

Executable is placed in build/bin with naming format NVim-Nightly-${COMMIT_DATE}-git${GIT_HASH_SUMMARY}.glibc${GLIBC_VERSION}-${ARCHITECTURE}.AppImage.

Name format is now Neovim-${COMMIT_DATE}-${NEOVIM_VERSION}-glibc${GLIBC_VERSION}-${ARCHITECTURE}.AppImage.

You can find pre-build executables to test here.

export ARCH="$(arch)"
fi
APP=NVim

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

Do not remember Neovim being named NVim. Why not Neovim?

# get_desktopintegration "$LOWERAPP"
########################################################################
# Determine the version of the app; also include needed glibc version

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

Where is “needed glibc version”?

This comment has been minimized.

@AdnoC

AdnoC May 2, 2017

Contributor

Ah, I need to change that comment. It used to add glibc to the version string twice - once there and once in the generate_appimage function in https://github.com/probonopd/AppImages/blob/master/functions.sh

@@ -0,0 +1,143 @@
#!/bin/bash

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

You are using sh scripts/genappimage.sh in Makefile. This can’t be #!/bin/bash in this case, sh is not bash in Ubuntu, it is dash.

fi
APP=NVim
LOWERAPP="${APP,,}"

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

No need to use weird bash features here, can simply hardcode a string. Though I guess this needs to be nvim and not tolower($APP).

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

It also looks like the only place you need bash, except that I do not know what appimage_functions.sh requires.

#get_apprun
# get_desktop
find "${ROOT_DIR}" -name "${LOWERAPP}.desktop" -xdev -exec cp {} "${LOWERAPP}.desktop" \;

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

I can hardly understand what find is doing here. Why not simply use a path?

# Since neovim increments versions slower than vim, using
# the commit's date makes more sense.
VIM_VER="$(date -d "@$(git log -1 --format=%ct)" "+%F")"

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

Should be NVIM_VER. And why not first include a base version (like 0.2.0) and then a date and not only a date? Also git knows how to format a date: git show --no-patch --date='format:%F' --format='%cd'.

cd "$APP_BUILD_DIR"
wget -q https://github.com/probonopd/AppImages/raw/master/functions.sh -O ./appimage_functions.sh

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

Our other scripts all use curl. Though most portable would be using cmake -DURL=https://github.com/probonopd/AppImages/raw/master/functions.sh -DFILE=./appimage_functions.sh cmake/Download.cmake, but it overly verbose.

#sed -i -e "s|/usr/local/|$APPDIR/usr/|g" usr/bin/nvim
#sed -i -e "s|/usr/share/doc/vim/|$APPDIR/usr/share/doc/vim/|g" usr/bin/nvim
patch_strings_in_file "$APP_DIR"/usr/bin/nvim '/usr/local/nvim' '$APPDIR/usr/nvim'

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

Looks hacky, can’t you use some defines for that? CMAKE_INSTALL_PREFIX is defined via make CMAKE_EXTRA_FLAGS="-DCMAKE_INSTALL_PREFIX=…" if you want to use top-level Makefile.

# Now packaging it as an AppImage
########################################################################
find "${ROOT_DIR}" -name "nvim.apprun" -xdev -exec cp {} "$APP_DIR/AppRun" \;

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

Again, why find and not exact path?

@marvim marvim added the RFC label May 2, 2017

# I don't believe this line is needed since the referenced directories are empty.
# Uncomment if this changes.
# Value taken from https://github.com/probonopd/AppImageKit/blob/appimagetool/master/AppRun.c
# export LD_LIBRARY_PATH="$(printf "LD_LIBRARY_PATH=%s/usr/lib/:%s/usr/lib/i386-linux-gnu/:%s/usr/lib/x86_64-linux-gnu/:%s/usr/lib32/:%s/usr/lib64/:%s/lib/:%s/lib/i386-linux-gnu/:%s/lib/x86_64-linux-gnu/:%s/lib32/:%s/lib64/:%s" "$APPIMAGEDIR" "$APPIMAGEDIR" "$APPIMAGEDIR" "$APPIMAGEDIR" "$APPIMAGEDIR" "$APPIMAGEDIR" "$APPIMAGEDIR" "$APPIMAGEDIR" "$APPIMAGEDIR" "$APPIMAGEDIR" "$LD_LIBRARY_PATH")"

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

This is incorrect, see AppImage/AppImageKit#391.

#!/bin/sh
# This script is a replacement for the standard AppRun binary.
# The problem is that AppRun normally appends to PYTHONPATH.

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

It does not append, it prepends.

@justinmk justinmk referenced this pull request May 2, 2017

Merged

news: version 0.2 #151

appimage: Clean up build script
Changed generated executable name format. It is now:
NeoVim-Nightly-${COMMIT_DATE}-${NEOVIM_VERSION}-glibc${GLIBC_VERSION}-${ARCHITECTURE}.AppImage

Use the nvim.desktop file already in the runtime directory.
Use symlinks to the nvim executable instead of a wrapper script for AppRun.
Use paths when moving needed files instead of `find`ing them first.
`make install` to the build directory instead of using `DESTDIR` then moving them.
User `curl` instead of `wget`.

Script now shebangs to `/bin/sh` instead of `/bin/bash`.
export ARCH="$(arch)"
fi
APP=NeoVim

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

Official variant is Neovim. One capital.

cd "$APP_DIR"
# No need for a fancy script. AppRun can just be a symlink to nvim.
ln -s ./usr/bin/nvim AppRun

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

ln -s does not really care whether path symlink points to exist. So may just as well be ln -s usr/bin/nvim "$APP_DIR/AppRun", without any cds.

This comment has been minimized.

@AdnoC

AdnoC May 2, 2017

Contributor

After failing so many symlinks due to using relative paths, I should have realized that by now.

This comment has been minimized.

@ZyX-I

ZyX-I May 2, 2017

Contributor

The cd is necessary in an interactive session: not because ln will start to care, but because tab completion will be enabled and you have less probability of error. This does not apply in scripts.

@ZyX-I

ZyX-I approved these changes May 2, 2017

LGTM, except that I am not familiar with appimage.

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 2, 2017

I decided to test this on ubuntu-14.04.5-desktop-amd64.iso (Trusty). There was one issue due to something in the functions.sh script from AppImage using a syntax of function declaration not compatible with /bin/sh, but its an easy fix and I opened a pull request for it.

The generated AppImage ran properly and still worked when ran on archlinux-2017.04.01-x86_64.iso. The AppImage built on ArchLinux also ran without errors on Trusty.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented May 2, 2017

@AdnoC The docker is good for such tests, there are also ubuntu and archlinux images. And travis docker images in https://quay.io/organization/travisci. Unless you are going to mess up with kernel (which is shared with the host) it would normally be faster then whatever you could use iso images for.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented May 2, 2017

@AdnoC Why merging with master? AFAIK after last policy review you are supposed to

  1. Avoid either merging or rebasing if you do not have conflicts or do not need some recent features.
  2. For small PRs with easy rebasing (this one is this I think) rebase.
  3. Only otherwise merge.
@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 2, 2017

eh?

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 2, 2017

oh! oops. So thats what "Update branch" does. My bad. I'll force push to remove the merge.

@AdnoC AdnoC force-pushed the AdnoC:appimage branch from 74128af to c4dd182 May 2, 2017

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 2, 2017

RE: Docker

I hadn't realized that you could run Linux containers on Windows. TIL

# Determine the version of the app; For use in file filename
########################################################################
VERSION="Nightly-$COMMIT_DATE-$VIM_VER"

This comment has been minimized.

@justinmk

justinmk May 3, 2017

Member

remove Nightly- please. The version is enough, and for tagged versions we don't want "nightly" in the file name.

# rm -rf ./usr/share/doc || true
#rm -rf ./usr/bin/vim || true
# remove unneded links
# find ./usr/bin -type l \! -name "gvim" -delete || true

This comment has been minimized.

@justinmk

justinmk May 3, 2017

Member

is left over from vim's script? Is this a TODO or can we just delete this section?

This comment has been minimized.

@AdnoC

AdnoC May 3, 2017

Contributor

It was from vim's script and can be deleted

# #for package in *.deb; do
# # dpkg -x $package .
# #done
# #rm -f *.deb

This comment has been minimized.

@justinmk

justinmk May 3, 2017

Member

delete this section starting from line 41

#!/bin/sh
########################################################################
# Package the baries built on Travis-CI as an AppImage

This comment has been minimized.

@justinmk

@justinmk justinmk added this to the 0.2.1 milestone May 3, 2017

appimage: Clean up build script
Removed "Nightly-" from final executable name.
Removed unused and commented out lines from the original vim-appimage script.
Removed unneeded `mv` from `usr/local` to `usr`. We install directly to `usr` now.
Fixed spelling mistake.
@justinmk

This comment has been minimized.

Member

justinmk commented May 4, 2017

This is really cool, I tried one of your release images.

But when I run (on ubuntu 14.04) ./scripts/genappimage.sh locally, there are errors:

ninja: Entering directory `build'
[3/3] Install the project...
FAILED: cd /home/vagrant/neovim/build && /usr/bin/cmake -P cmake_install.cmake
-- Install configuration: "Debug"
-- Installing: /usr/local/share/man/man1/nvim.1
CMake Error at cmake_install.cmake:72 (FILE):
  file INSTALL cannot copy file "/home/vagrant/neovim/man/nvim.1" to
  "/usr/local/share/man/man1/nvim.1".


ninja: build stopped: subcommand failed.
make: *** [install] Error 1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   139  100   139    0     0    261      0 --:--:-- --:--:-- --:--:--   261
100 11364  100 11364    0     0  15789      0 --:--:-- --:--:-- --:--:-- 15789
./scripts/genappimage.sh: 43: cd: can't cd to Neovim.AppDir
./scripts/genappimage.sh: 67: ./scripts/genappimage.sh: [[: not found
./scripts/genappimage.sh: 67: ./scripts/genappimage.sh: [[: not found
./scripts/genappimage.sh: 67: ./scripts/genappimage.sh: [[: not found
./scripts/genappimage.sh: 67: ./scripts/genappimage.sh: [[: not found
./scripts/genappimage.sh: 67: ./scripts/genappimage.sh: [[: not found
./scripts/genappimage.sh: 67: ./scripts/genappimage.sh: [[: not found
ld-linux.so.2 ld-linux-x86-64.so.2 libanl.so.1 libBrokenLocale.so.1 libcidn.so.1 libcrypt.so.1 libc.so.6 libdl.so.2 libm.so.6 libmvec.so.1 libnsl.so.1 libns
s_compat.so.2 libnss_db.so.2 libnss_dns.so.2 libnss_files.so.2 libnss_hesiod.so.2 libnss_nisplus.so.2 libnss_nis.so.2 libpthread.so.0 libresolv.so.2 librt.s
o.1 libthread_db.so.1 libutil.so.1 libstdc++.so.6 libGL.so.1 libdrm.so.2 libxcb.so.1 libX11.so.6 libgio-2.0.so.0 libgdk-x11-2.0.so.0 libgtk-x11-2.0.so.0 lib
asound.so.2 libgdk_pixbuf-2.0.so.0 libfontconfig.so.1 libselinux.so.1 libcom_err.so.2 libcrypt.so.1 libexpat.so.1 libgcc_s.so.1 libglib-2.0.so.0 libgpg-erro
r.so.0 libgssapi_krb5.so.2 # Disputed, seemingly needed by Arch Linux since Kerberos is named differently there libhcrypto.so.4 libhx509.so.5 libICE.so.6 li
bidn.so.11 libk5crypto.so.3 libkeyutils.so.1 libkrb5.so.26 # Disputed, seemingly needed by Arch Linux since Kerberos is named differently there libkrb5.so.3
 # Disputed, seemingly needed by Arch Linux since Kerberos is named differently there libkrb5support.so.0 # Disputed, seemingly needed by Arch Linux since K
erberos is named differently there libp11-kit.so.0 libroken.so.18 libSM.so.6 libusb-1.0.so.0 libuuid.so.1 libwind.so.0 libz.so.1 libgobject-2.0.so.0 libgpg-
error.so.0
ln: failed to access ‘/home/vagrant/neovim/build/Neovim.AppDir/AppRun’: Not a directory
--2017-05-03 21:45:00--  https://github.com/probonopd/AppImageKit/releases/download/6/AppImageAssistant_6-x86_64.AppImage
Resolving github.com (github.com)... 192.30.253.112, 192.30.253.113
Connecting to github.com (github.com)|192.30.253.112|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://github-cloud.s3.amazonaws.com/releases/9435153/d4fbde64-7778-11e6-9e10-37eb60fc42de.AppImage?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAISTNZFOVBIJMK3TQ%2F20170504%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20170504T095841Z&X-Amz-Expires=300&X-Amz-Signature=ba20a2b09bc1dbc112f9af8fa1b7746664ce8610e974ed86cf0f67906087b138&X-Amz-SignedHeaders=host&actor_id=0&response-content-disposition=attachment%3B%20filename%3DAppImageAssistant_6-x86_64.AppImage&response-content-type=application%2Foctet-stream [following]
--2017-05-03 21:45:00--  https://github-cloud.s3.amazonaws.com/releases/9435153/d4fbde64-7778-11e6-9e10-37eb60fc42de.AppImage?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAISTNZFOVBIJMK3TQ%2F20170504%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20170504T095841Z&X-Amz-Expires=300&X-Amz-Signature=ba20a2b09bc1dbc112f9af8fa1b7746664ce8610e974ed86cf0f67906087b138&X-Amz-SignedHeaders=host&actor_id=0&response-content-disposition=attachment%3B%20filename%3DAppImageAssistant_6-x86_64.AppImage&response-content-type=application%2Foctet-stream
Resolving github-cloud.s3.amazonaws.com (github-cloud.s3.amazonaws.com)... 52.216.225.160
Connecting to github-cloud.s3.amazonaws.com (github-cloud.s3.amazonaws.com)|52.216.225.160|:443... connected.
HTTP request sent, awaiting response... 416 Requested Range Not Satisfiable

    The file is already fully retrieved; nothing to do.

['/tmp/.mount_YnLLfX/package', './Neovim.AppDir/', '../out/Neovim-v0.2.1-26-g1a09cfe95153.glibc2.17-x86_64.AppImage']
Icon could not be found based on information in desktop file, aborting
mv: cannot stat ‘/home/vagrant/neovim/out/*.AppImage’: No such file or directory

Also, it seems to expect nvim to be built before running. Nope, that was my mistake.

I'm trying to gather all the steps needed to automate this on CI.

@justinmk

This comment has been minimized.

Member

justinmk commented May 4, 2017

The ]] not found errors are probably because sh on ubuntu is not bash. Changing the shebang to bin/bash fixes that issue.

Is ../scripts/genappimage.sh expecting to be run from within the build/ directory?

@justinmk

This comment has been minimized.

Member

justinmk commented May 4, 2017

Do not remember Neovim being named NVim. Why not Neovim?

The APP=Neovim name is used for the runnable binary name. We teach users to run nvim, not neovim, so it should be APP=nvim. The result will look like nvim-{version}.AppImage.

justinmk added a commit to justinmk/neovim that referenced this pull request May 4, 2017

dist: AppImage neovim#6638
scripts/genappimage.sh produces an executable:
    Neovim-${COMMIT_DATE}-${NEOVIM_VERSION}-glibc${GLIBC_VERSION}-${ARCHITECTURE}.AppImage

- Use the nvim.desktop file already in the runtime directory.
- Use symlinks to the nvim executable instead of a wrapper script for
  AppRun.
- Use paths when moving needed files instead of `find`ing them first.
  `make install` to the build directory instead of using `DESTDIR` then
  moving them.
- Install directly to usr/.

Closes neovim#6083

@justinmk justinmk referenced this pull request May 4, 2017

Closed

dist: AppImage #6676

justinmk added a commit to justinmk/neovim that referenced this pull request May 4, 2017

dist: AppImage neovim#6638
scripts/genappimage.sh produces an executable:
    Neovim-${COMMIT_DATE}-${NEOVIM_VERSION}-glibc${GLIBC_VERSION}-${ARCHITECTURE}.AppImage

- Use the nvim.desktop file already in the runtime directory.
- Use symlinks to the nvim executable instead of a wrapper script for
  AppRun.
- Use paths when moving needed files instead of `find`ing them first.
  `make install` to the build directory instead of using `DESTDIR` then
  moving them.
- Install directly to usr/.

Closes neovim#6083

justinmk added a commit that referenced this pull request May 4, 2017

dist: AppImage #6638
scripts/genappimage.sh produces an executable:
    nvim-${NVIM_VERSION}-glibc${GLIBC_VERSION}-${ARCHITECTURE}.AppImage

Closes #6083
@justinmk

This comment has been minimized.

Member

justinmk commented May 4, 2017

Merged. Thank you @AdnoC and @ZyX-I. And thanks to @probonopd -- I'm really impressed by AppImage!

@justinmk justinmk closed this May 4, 2017

@justinmk justinmk removed the RFC label May 4, 2017

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 4, 2017

I'm not sure why it would say ]] not found on Ubuntu 14.04, as I didn't get that when I ran it on a clean install.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment