Some packaging concerns

natrys commented 3 years ago

Hello, first of all, thanks for this program. It seems to work nicely.

I wanted to package it using my distro's native packaging mechanism so that maybe in future it could be incorporated in official repository. So I wanted to open a dialogue about some issues that arose, two mostly small ones, one maybe a bit more complicated.

1) Most distros have official policy not to allow ELF files in /usr/share/, please consider putting the binaries in appropriate place like /usr/bin/.

2) The source tarball generated by github doesn't contain submodules. This is a long-standing github issue, but in any case distros expect fetched source distfiles to be self-containing and git is generally not desirable to be involved. However, submodules can be tricky to handle even with git-archive. I played with your github actions pipeline, and the following patch that uses a 3rd party tool worked for me. Please review, if you agree this is worth fixing in this way:

diff --git .github/workflows/deploy.yml .github/workflows/deploy.yml
index 2fc692a..522b85a 100644
--- .github/workflows/deploy.yml
+++ .github/workflows/deploy.yml
@@ -70,12 +70,23 @@ jobs:
     steps:
       - name: checkout-source
         uses: actions/checkout@v1
+        with:
+          submodules: recursive
       - name: get-version
         id: get_version
         shell: bash
         run: |
           RELEASE_VERSION=$(cat version.txt | head -n1)
           echo ::set-output name=RELEASE_VERSION::$RELEASE_VERSION
+      - uses: actions/setup-python@v2
+      - name: create tarball
+        run: |
+          pip install git-archive-all
+          TARBALL="$(basename $(git rev-parse --show-toplevel))-${VERSION}.tar.gz"
+          mkdir -p artifact/
+          git-archive-all artifact/${TARBALL}
+        env:
+          VERSION: ${{ steps.get_version.outputs.RELEASE_VERSION }}
       - name: download-artifacts
         uses: actions/download-artifact@v1
         with:

3) However the biggest issue I want to raise is that cross-compilation seems broken/unsupported. This should be solvable because individually both cmake and cargo has cross-compiling support. But something seems lost in the integration because building riti fails for me when cross-compiling from x86-64 for target aarch64.

Ability to cross-compile cleanly is important prerequisite for a package to be officially accepted in the repo. Please consider including that ability within the scope of this project.

bdeshi commented 3 years ago

pt2: so the request is to include a self-contained source package in the releases filelist. can't we just zip up the source directory after checkout to avoid installing python and python packages? we will have a couple non-essential files in the archive; anything else?

natrys commented 3 years ago

Yes, zipping would work as well but non-essential files will include git related stuffs (all .git folders in the tree etc) which can bloat up the source archive considerably (doing 7z/zip on the folder produces 25M archive which I think is very unreasonable for just source). That said, it seems if you checkout at depth 1, and use tar+gzip, result is 8M or so which I guess is agreeable.

mominul commented 3 years ago

Hi, sorry for being so late! :confounded:

1. Most distros have official policy not to allow ELF files in `/usr/share/`, please consider putting the binaries in appropriate place like `/usr/bin/`.
I'll address this issue in the next version.

The source tarball generated by github doesn't contain submodules. This is a long-standing github issue, but in any case distros expect fetched source distfiles to be self-containing and git is generally not desirable to be involved. However, submodules can be tricky to handle even with git-archive. I played with your github actions pipeline, and the following patch that uses a 3rd party tool worked for me. Please review, if you agree this is worth fixing in this way: I don't want to continue using the submodule feature for long. In the meantime how about including a source tar file built using the tool you have proposed in the releases?

However the biggest issue I want to raise is that cross-compilation seems broken/unsupported. This should be solvable because individually both cmake and cargo has cross-compiling support. But something seems lost in the integration because building riti fails for me when cross-compiling from x86-64 for target aarch64. I am not quite familiar with the cross compilation process, but I am really interested to figure out the issue together! Can you please post the errors you are having here?

Thanks! :blush:

natrys commented 3 years ago

In the meantime how about including a source tar file built using the tool you have proposed in the releases?

Sure, if you are okay with manually adding it, that would work for me too. But don't feel compelled to do it before tackling the other issues, because this is probably not going to be useful to anyone other than maintainers or potential packagers.

Can you please post the errors you are having here?

Sure:

-- The C compiler identification is GNU 9.3.0
-- The CXX compiler identification is GNU 9.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /builddir/.xbps-OpenBangla-Keyboard/wrappers/aarch64-linux-gnu-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /builddir/.xbps-OpenBangla-Keyboard/wrappers/aarch64-linux-gnu-c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Cargo Home: /tmp/.cargo
-- Rust Compiler Version: rustc 1.46.0
-- Found PkgConfig: aarch64-linux-gnu-pkg-config (found version "0.29.2") 
-- Checking for module 'ibus-1.0'
--   Found ibus-1.0, version 1.5.22
-- Checking for module 'libzstd'
--   Found libzstd, version 1.4.5
-- Configuring done
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_INSTALL_LIBDIR
    CMAKE_INSTALL_SBINDIR

Scanning dependencies of target libShared
[  3%] Building CXX object src/shared/CMakeFiles/libShared.dir/Settings.cpp.o
[  7%] Building CXX object src/shared/CMakeFiles/libShared.dir/FileSystem.cpp.o
[ 10%] Linking CXX static library liblibShared.a
[ 10%] Built target libShared
Scanning dependencies of target riti_target
[ 14%] running cargo
    Updating crates.io index
 Downloading crates ...
  Downloaded itoa v0.4.6
  Downloaded lazy_static v1.4.0
  Downloaded cfg-if v1.0.0
  Downloaded ahash v0.3.8
  Downloaded const_fn v0.4.3
  Downloaded rayon-core v1.9.0
  Downloaded crossbeam-deque v0.8.0
  Downloaded either v1.6.1
  Downloaded crossbeam-epoch v0.9.1
  Downloaded crossbeam-utils v0.8.1
  Downloaded edit-distance v2.1.0
  Downloaded crossbeam-channel v0.5.0
  Downloaded aho-corasick v0.7.15
  Downloaded scopeguard v1.1.0
  Downloaded memoffset v0.6.1
  Downloaded memchr v2.3.4
  Downloaded num_cpus v1.13.0
  Downloaded hashbrown v0.8.2
  Downloaded serde v1.0.117
  Downloaded serde_json v1.0.60
  Downloaded rupantor v0.3.0
  Downloaded stringplus v0.1.0
  Downloaded thread_local v1.0.1
  Downloaded regex v1.4.2
  Downloaded regex-syntax v0.6.21
  Downloaded libc v0.2.80
  Downloaded rayon v1.5.0
  Downloaded autocfg v1.0.1
  Downloaded ryu v1.0.5
   Compiling autocfg v1.0.1
   Compiling lazy_static v1.4.0
   Compiling cfg-if v1.0.0
   Compiling const_fn v0.4.3
error[E0463]: can't find crate for `core`

error[E0463]: can't find crate for `core`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.
error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.
error: could not compile `cfg-if`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
make[2]: *** [src/engine/riti/CMakeFiles/riti_target.dir/build.make:99: src/engine/riti/x86_64-unknown-linux-gnu/release/libriti.a] Error 101
make[1]: *** [CMakeFiles/Makefile2:272: src/engine/riti/CMakeFiles/riti_target.dir/all] Error 2
make: *** [Makefile:171: all] Error 2

Fairly certain this is cross-compilation related.

I am not quite familiar with the cross compilation process, but I am really interested to figure out the issue together!

Well I can't claim to be an expert in this area either. And usually people don't need to be, build systems and proper toolchains are supposed to abstract this away (had this been an only C++ or Rust project, cmake or cargo would have probably taken care of it). This situation is slightly exacerbated by the fact that cross-compilation is out of scope for some popular distros like Arch because they are only concerned with single architecture (x86_64). But more ambitious distros like Debian/Gentoo (or Void in my usecase) supports at least flavours of ARMs, and then more. Had we been been building this software natively in such machines, again this likely wouldn't have caused issues. But unfortunately native infrastructure powerful enough to operate continuous building and testing for all packages would be fairly expensive to maintain. Which is why cross-compilation exists and is important, you can use your regular x86_64 build server to cater to even raspberry pi users.

Unfortunately this does introduce a few more new complications. You need to install special architecture specific flavour of compiler toolchain/libc as well as setup variables and flags for them in particular manners. Here is my stab regarding what happened (and I could be off base):

Distro helper scripts correctly detected I am cross-compiling, and installed proper toolchain (cross-aarch64-linux-gnu, cross-aarch64-linux-gnu-libc)
Distro helper scripts also hinted cmake properly compiler variables and flags, and more importantly set CMAKE_CROSSCOMPILING to true, CMAKE_SYSTEM_PROCESSOR to aarch64 and CMAKE_FIND_ROOT_PATH to /usr/aarch64-linux-gnu/ (this is the path where aarch64 version of all dependencies like ibus and qt5 are installed)
And now we can see that cmake detects the cross-compiler correctly (e.g. the wrapper for CC eventually resolves to /usr/bin/aarch64-linux-gnu-gcc) without any interventions from your part

So far so good it seems. What about when cmake hands it over to Rust? Cargo also needs to know we are cross-compiling because at the very least, the rust-std is architecture specific, so it can't use the x86_64 version available in /usr/lib/rustlib/x86_64-unknown-linux-gnu/ rather needs to know the correct location of it is (which would be /usr/aarch64-linux-gnu/usr/lib/rustlib/aarch64-unknown-linux-gnu/), and secondly Rust still seems to rely on system linker so cargo needs to know where the cross-compiler toolchain is. So, the correct cargo invocation for cross-compiling a simple program from x86_64 to aarch64 (given above configuration) would need to be at least something like:

export CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER=/usr/bin/aarch64-linux-gnu-gcc
export RUSTFLAGS=--sysroot=/usr/aarch64-linux-gnu/

cargo build --release --target=aarch64-unknown-linux-gnu

But is this what's happening here? Let's look at the verbose log:

-- The C compiler identification is GNU 9.3.0
-- The CXX compiler identification is GNU 9.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /builddir/.xbps-OpenBangla-Keyboard/wrappers/aarch64-linux-gnu-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /builddir/.xbps-OpenBangla-Keyboard/wrappers/aarch64-linux-gnu-c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Cargo Home: /host/cargo
-- Rust Compiler Version: rustc 1.46.0
-- Found PkgConfig: aarch64-linux-gnu-pkg-config (found version "0.29.2") 
-- Checking for module 'ibus-1.0'
--   Found ibus-1.0, version 1.5.22
-- Checking for module 'libzstd'
--   Found libzstd, version 1.4.5
-- Configuring done
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_INSTALL_LIBDIR
    CMAKE_INSTALL_SBINDIR

/usr/bin/cmake -S/builddir/OpenBangla-Keyboard-2.0.0 -B/builddir/OpenBangla-Keyboard-2.0.0/build --check-build-system CMakeFiles/Makefile.cmake 0
/usr/bin/cmake -E cmake_progress_start /builddir/OpenBangla-Keyboard-2.0.0/build/CMakeFiles /builddir/OpenBangla-Keyboard-2.0.0/build//CMakeFiles/progress.marks
make  -f CMakeFiles/Makefile2 all
make[1]: Entering directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
make  -f src/shared/CMakeFiles/libShared.dir/build.make src/shared/CMakeFiles/libShared.dir/depend
make[2]: Entering directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
cd /builddir/OpenBangla-Keyboard-2.0.0/build && /usr/bin/cmake -E cmake_depends "Unix Makefiles" /builddir/OpenBangla-Keyboard-2.0.0 /builddir/OpenBangla-Keyboard-2.0.0/src/shared /builddir/OpenBangla-Keyboard-2.0.0/build /builddir/OpenBangla-Keyboard-2.0.0/build/src/shared /builddir/OpenBangla-Keyboard-2.0.0/build/src/shared/CMakeFiles/libShared.dir/DependInfo.cmake --color=
Scanning dependencies of target libShared
make[2]: Leaving directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
make  -f src/shared/CMakeFiles/libShared.dir/build.make src/shared/CMakeFiles/libShared.dir/build
make[2]: Entering directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
[  3%] Building CXX object src/shared/CMakeFiles/libShared.dir/Settings.cpp.o
cd /builddir/OpenBangla-Keyboard-2.0.0/build/src/shared && /builddir/.xbps-OpenBangla-Keyboard/wrappers/aarch64-linux-gnu-c++ -DPROJECT_DATADIR=\"/usr/share/openbangla-keyboard\" -DPROJECT_VERSION=\"2.0.0\" -DQT_CORE_LIB -DQT_NO_DEBUG -I /usr/aarch64-linux-gnu/usr/include/qt5 -I /usr/aarch64-linux-gnu/usr/include/qt5/QtCore -I /usr/aarch64-linux-gnu/usr/../usr/lib/qt5/mkspecs/linux-g++ -fstack-clash-protection -D_FORTIFY_SOURCE=2 -O2 -march=armv8-a   -I/usr/aarch64-linux-gnu/usr/include -O3 -DNDEBUG -fPIC -std=gnu++11 -o CMakeFiles/libShared.dir/Settings.cpp.o -c /builddir/OpenBangla-Keyboard-2.0.0/src/shared/Settings.cpp
[  7%] Building CXX object src/shared/CMakeFiles/libShared.dir/FileSystem.cpp.o
cd /builddir/OpenBangla-Keyboard-2.0.0/build/src/shared && /builddir/.xbps-OpenBangla-Keyboard/wrappers/aarch64-linux-gnu-c++ -DPROJECT_DATADIR=\"/usr/share/openbangla-keyboard\" -DPROJECT_VERSION=\"2.0.0\" -DQT_CORE_LIB -DQT_NO_DEBUG -I /usr/aarch64-linux-gnu/usr/include/qt5 -I /usr/aarch64-linux-gnu/usr/include/qt5/QtCore -I /usr/aarch64-linux-gnu/usr/../usr/lib/qt5/mkspecs/linux-g++ -fstack-clash-protection -D_FORTIFY_SOURCE=2 -O2 -march=armv8-a   -I/usr/aarch64-linux-gnu/usr/include -O3 -DNDEBUG -fPIC -std=gnu++11 -o CMakeFiles/libShared.dir/FileSystem.cpp.o -c /builddir/OpenBangla-Keyboard-2.0.0/src/shared/FileSystem.cpp
[ 10%] Linking CXX static library liblibShared.a
cd /builddir/OpenBangla-Keyboard-2.0.0/build/src/shared && /usr/bin/cmake -P CMakeFiles/libShared.dir/cmake_clean_target.cmake
cd /builddir/OpenBangla-Keyboard-2.0.0/build/src/shared && /usr/bin/cmake -E cmake_link_script CMakeFiles/libShared.dir/link.txt --verbose=1
/usr/bin/aarch64-linux-gnu-ar qc liblibShared.a CMakeFiles/libShared.dir/Settings.cpp.o CMakeFiles/libShared.dir/FileSystem.cpp.o
/usr/bin/aarch64-linux-gnu-ranlib liblibShared.a
make[2]: Leaving directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
[ 10%] Built target libShared
make  -f src/engine/riti/CMakeFiles/riti_target.dir/build.make src/engine/riti/CMakeFiles/riti_target.dir/depend
make[2]: Entering directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
cd /builddir/OpenBangla-Keyboard-2.0.0/build && /usr/bin/cmake -E cmake_depends "Unix Makefiles" /builddir/OpenBangla-Keyboard-2.0.0 /builddir/OpenBangla-Keyboard-2.0.0/src/engine/riti /builddir/OpenBangla-Keyboard-2.0.0/build /builddir/OpenBangla-Keyboard-2.0.0/build/src/engine/riti /builddir/OpenBangla-Keyboard-2.0.0/build/src/engine/riti/CMakeFiles/riti_target.dir/DependInfo.cmake --color=
Scanning dependencies of target riti_target
make[2]: Leaving directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
make  -f src/engine/riti/CMakeFiles/riti_target.dir/build.make src/engine/riti/CMakeFiles/riti_target.dir/build
make[2]: Entering directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
[ 14%] running cargo
cd /builddir/OpenBangla-Keyboard-2.0.0/src/engine/riti && /usr/bin/cmake -E env CARGO_TARGET_DIR=/builddir/OpenBangla-Keyboard-2.0.0/build/src/engine/riti /usr/bin/cargo build --target x86_64-unknown-linux-gnu --release
    Updating crates.io index
   Compiling autocfg v1.0.1
   Compiling lazy_static v1.4.0
error[E0463]: can't find crate for `core`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.
error: could not compile `lazy_static`.

To learn more, run the command again with --verbose.
make[2]: *** [src/engine/riti/CMakeFiles/riti_target.dir/build.make:102: src/engine/riti/x86_64-unknown-linux-gnu/release/libriti.a] Error 101
make[2]: Leaving directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
make[1]: *** [CMakeFiles/Makefile2:275: src/engine/riti/CMakeFiles/riti_target.dir/all] Error 2
make[1]: Leaving directory '/builddir/OpenBangla-Keyboard-2.0.0/build'
make: *** [Makefile:174: all] Error 2

So something like this shows cmake is fairly well aware:

-DPROJECT_DATADIR=\"/usr/share/openbangla-keyboard\" -DPROJECT_VERSION=\"2.0.0\" -DQT_CORE_LIB -DQT_NO_DEBUG -I /usr/aarch64-linux-gnu/usr/include/qt5 -I /usr/aarch64-linux-gnu/usr/include/qt5/QtCore -I /usr/aarch64-linux-gnu/usr/../usr/lib/qt5/mkspecs/linux-g++ -fstack-clash-protection -D_FORTIFY_SOURCE=2 -O2 -march=armv8-a   -I/usr/aarch64-linux-gnu/usr/include -O3 -DNDEBUG -fPIC -std=gnu++11 -o CMakeFiles/libShared.dir/Settings.cpp.o -c

But as for cargo:

cd /builddir/OpenBangla-Keyboard-2.0.0/src/engine/riti && /usr/bin/cmake -E env CARGO_TARGET_DIR=/builddir/OpenBangla-Keyboard-2.0.0/build/src/engine/riti /usr/bin/cargo build --target x86_64-unknown-linux-gnu --release

Particularly the --target x86_64-unknown-linux-gnu part shows that cargo has no idea that we are cross-compiling at all.

So at this point it calls for closer look at your integration between cmake and cargo. In particular if we look at the file where cargo_build is defined, seems like it's setting LIB_TARGET very naively and not in a cross-compilation aware way.

So I guess my diagnosis is, you probably need to use another cmake plugin for cargo which supports cross-compilation, because current one doesn't seem to (but I could be wrong).

natrys commented 3 years ago

Okay upon another look, turns out that one way to fix this is simply to do:

if(CMAKE_CROSSCOMPILING)
    set(LIB_TARGET $ENV{RUST_TARGET})
endif()

As for why this works, my distro helper scripts already helpfully defined $RUSTFLAGS, $RUST_TARGET and $RUST_BUILD in the environment. These are inherited by cmake, and it's passing them to cargo. So the environment is correct, I just had to override the erroneously set LIB_TARGET in cross-compilation context with correct value passed via environment. It shouldn't affect anything else otherwise. The linker location is still missing, but here it's a static library so linker is irrelevant.

I am not sure whether this would work in other distros (common sense says it should) but I just tested my Void template for aarch64 and armv6l-musl and it built correctly (though can't test if it would actually work, for lack of proper hardware).

I also forgot to address this:

I don't want to continue using the submodule feature for long.

The submodules feature however gives packagers an extra option in that, if my above approach doesn't work i.e. then people could package riti and this program separately (with riti being a dependency of this). It would be modular that way (debian people love splitting stuffs). However you would need to change build system to look for riti first in default system library location (via pkg-config) for the packagers and then fallback to local folder for your local development. But that's a different conversation.

mominul commented 3 years ago

Thanks for your detailed investigation @natrys!

Okay upon another look, turns out that one way to fix this is simply to do:

I have no problem with accepting this kind of build system hackery, as the CMakeRust module I used here doesn't support cross compilation like you mentioned earlier. Would you like to open a PR?

Actually I have the intention to rewrite the engine part in Rust and let cargo to handle the rest, so using submodule is not the way forward in my mind currently. :wink: I am quite unaware of the packaging strategies of Debian and other distros, so I'd like to know about them and get OBK packaged in the official repositories! But the build system is in quite a messy situation for cross compilation support currently. :disappointed: Which might be a requirement for Debian I suspect?

Thanks!

bdeshi commented 3 years ago

I am quite unaware of the packaging strategies of Debian and other distros, so I'd like to know about them and get OBK packaged in the official repositories!

@mominul for debian, we need to make a request to the packaging team for inclusion in the official repos. if it's accepted, a mentor can help with the process. they can provide build/mirror/hosting infrastructure as well (salsa, autobuilder etc).

(in fact, becoming a part of debian offical repo can be a huge boost for this project's usage. I think we can track this on a separate issue.)

I don't think deb specifically requires cross-compilation, but having the capability will not harm at all!

natrys commented 3 years ago

Sent a PR. Meanwhile I will await for next release to address first two points, before I make my PR over to Void.

mominul commented 3 years ago

With #206 the only issue remaining is the source tarball generated by GitHub doesn't contain submodules.

OpenBangla / OpenBangla-Keyboard

Some packaging concerns #183