Skip to content
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

Refactor GitHub CD workflow #992

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

jonkerj
Copy link

@jonkerj jonkerj commented Aug 27, 2021

A long time ago, I started working on #653. I found it hard to find the time to improve the CD, but in the previous week I managed to finish it. The original PR has gone stale/rotten/closed so I'm filing this new one.

This PR achieves a couple of things:

  • No messy individual deb-extracts: it leverages multiarch
  • Arm64 support (fixes Provide arm binaries for all platforms.  #280)
  • Consistent artifact naming (ie, spotifyd-<os>-<arch>-<type>)
  • Consistent feature set (linux/slim is always alsa)
  • Almost no DRY

There are two drawbacks I am not sure about:

  • I did not include armv6. Is this really used?
  • The linux artifact types had different feature set (specifically audio backends) across architectures: x86_64 used pulseaudio, even in slim, but arm did not. Was this intentional?

I've tested this in my repo, and have a full artifact set for v0.3.2-cd.3.

robinvd
robinvd previously approved these changes Sep 21, 2021
Copy link
Contributor

@robinvd robinvd left a comment

Choose a reason for hiding this comment

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

Cool! indeed much more DRY. I didnt test it, but if you say it works on your branch then that should be fine.

@MichaIng
Copy link

I did not include armv6. Is this really used?

Yes it is, Raspberry Pi 1 and Raspberry Pi Zero models (which are still heavily used), so would be great to keep it. Alternatively the ARMv7 binaries could be replaced with ARMv6 ones (could be called "armhf" or "arm32"), which works well due to backwards compatibility. But ARMv6 binaries are usually larger (= higher memory usage) and possibly have other minor downsides on ARMv7 capable systems, so it isn't optional, just common practice elsewhere 😉.

@jonkerj
Copy link
Author

jonkerj commented Oct 1, 2021

That would require a bit of re-thinking, but if it is used, it should work. I'll look into it.

@jonkerj
Copy link
Author

jonkerj commented Oct 19, 2021

Small update: I've been fighting with toolchains for a couple of days now, and supporting armv6 seems very hard. Default toolchains on recent ubuntu (20.04) is unable to build armv6/thumb-1 binaries and third party toolchains are very clunky to use (ignoring the supply chain attack vector here).

In order to safely build armv6 binaries on recent ubuntu, you'll need to roll your own toolchain (yay..) or trust a third party.

@MichaIng
Copy link

gcc-arm-linux-gnueabihf compiler with --with-arch=armv6 --with-float=hard --with-fpu=vfp flags does not work?

@jonkerj
Copy link
Author

jonkerj commented Oct 19, 2021

The packages gcc-{8,9,10}-arm-linux-gnueabihf are not compiled "with" armv6/thumb-1, etc:

jorik@zinc:$ arm-linux-gnueabihf-gcc -mcpu=arm1176jzf-s -mfloat-abi=hard -mfpu=vfp /tmp/silly.c -o /tmp/silly
/tmp/silly.c: In function ‘main’:
/tmp/silly.c:1:1: sorry, unimplemented: Thumb-1 hard-float VFP ABI
    1 | int main() {}
      | ^~~
jorik@zinc:$ arm-linux-gnueabihf-gcc -v
Using built-in specs.
COLLECT_GCC=arm-linux-gnueabihf-gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc-cross/arm-linux-gnueabihf/9/lto-wrapper
Target: arm-linux-gnueabihf
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 9.3.0-17ubuntu1~20.04' --with-bugurl=file:///usr/share/doc/gcc-9/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,gm2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-9 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libitm --disable-libquadmath --disable-libquadmath-support --enable-plugin --enable-default-pie --with-system-zlib --without-target-system-zlib --enable-libpth-m2 --enable-multiarch --enable-multilib --disable-sjlj-exceptions --with-arch=armv7-a --with-fpu=vfpv3-d16 --with-float=hard --with-mode=thumb --disable-werror --enable-multilib --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=arm-linux-gnueabihf --program-prefix=arm-linux-gnueabihf- --includedir=/usr/arm-linux-gnueabihf/include
Thread model: posix
gcc version 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04)

I'm going to do yet another attempt using bootlin's precompiled toolchains. These are for specific archs (in this case armv6/eabihf/glibc), up to date, and reasonably trustworthy.

@MichaIng
Copy link

Here is the official toolchain: https://github.com/raspberrypi/tools
But it states to be deprecated and suggests what I posted above. What generally works is compiling on a Raspbian system, e.g. booted/chrooted as QEMU container.

@jonkerj
Copy link
Author

jonkerj commented Oct 21, 2021

OK, I can build a binary using a armv6 toolchain (bootlin's armv6/glibc). When I readelf -A the resulting binary, it still shows wrong arch, even though the toolchain is not able to -march=armv7:

jorik@zinc:~/spotifyd/spotifyd$ readelf -A target/arm-unknown-linux-gnueabihf/release/spotifyd
Attribute Section: aeabi
File Attributes
  Tag_CPU_name: "7-A"
  Tag_CPU_arch: v7
  Tag_CPU_arch_profile: Application
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-2
  Tag_FP_arch: VFPv3-D16
[..]
jorik@zinc:~/spotifyd/spotifyd$ echo 'int main() {}' > ../silly.c
jorik@zinc:~/spotifyd/spotifyd$ arm-buildroot-linux-gnueabihf-gcc -march=armv7 -o ../silly ../silly.c
cc1: error: target CPU does not support ARM mode

Interesting thing is that all intermediate objects are armv6/thumb1:

jorik@zinc:~/spotifyd/spotifyd$ readelf -A target/arm-unknown-linux-gnueabihf/release/deps/libtextwrap-ba6f73e7c9969203.rlib

File: target/arm-unknown-linux-gnueabihf/release/deps/libtextwrap-ba6f73e7c9969203.rlib(textwrap-ba6f73e7c9969203.textwrap.7dc394a6-cgu.0.rcgu.o)
Attribute Section: aeabi
File Attributes
  Tag_conformance: "2.09"
  Tag_CPU_arch: v6
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-1
  Tag_FP_arch: VFPv2
  Tag_ABI_PCS_R9_use: V6

Since the compiler refuses to create armv7 binaries, this makes me think it's the linker causing this issue. Maybe that is because of libc6-dev:armhf contains several static objects (.a files) which are getting linked, and being armv7/thumb2:

File: /usr/lib/arm-linux-gnueabihf/libm.a(s_lib_version.o)
Attribute Section: aeabi
File Attributes
  Tag_CPU_name: "7-A"
  Tag_CPU_arch: v7
  Tag_CPU_arch_profile: Application
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-2

So I need to think of an elegant solution to download the build/link dependencies; they probably need to be really old.

@tomodachi
Copy link
Contributor

@jonkerj
I tested the a release from your repository assuming it's built using this CD workflow.
With the relase spotifyd-linux-armhf-full I get :

"./spotifyd: /lib/arm-linux-gnueabihf/libm.so.6: version `GLIBC_2.29' not found (required by ./spotifyd)"

On a PI 3 with Rasbian (Debian Buster)

The same release from spotifyd repository works fine.

@MichaIng
Copy link

It would need to be linked against an older glibc version to be compatible with older OS versions. Debian/Raspbian Buster ships v2.28, and it is the most used distro version on armhf SBCs, so this makes sense as a minimum, I'd say. Ubuntu Bionic ships glibc v2.27.

@slondr
Copy link
Member

slondr commented Mar 11, 2022

I'm not sure if supporting first-generation RPis in our CD specifically is worth delaying the other changes here.

slondr
slondr previously approved these changes Mar 11, 2022
@slondr slondr requested a review from robinvd March 11, 2022 02:35
robinvd
robinvd previously approved these changes Mar 11, 2022
@tomodachi
Copy link
Contributor

tomodachi commented Mar 23, 2022

Hmm maybe it wasn't clear from my comment, but this didn't work on a rapsberrypi-3 so it's not only first-gen raspberrys that will will not work with this.

@slondr
Copy link
Member

slondr commented Sep 28, 2022

Oh huh does this actually bring back armv6 support now?

@slondr slondr requested a review from robinvd September 28, 2022 23:56
@robinvd
Copy link
Contributor

robinvd commented Sep 29, 2022

@slondr its a little too big for me to fully review right now. If you tested it and looks look then go ahead

Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

I did some testing with this PR, and it seems that it doesn't currently work as expected.

  1. The CD doesn't currently build, it apparently just starts a debug shell (?) on the prepared system.
  2. full fails for all non-x86_64 architectures.

I'll try to investigate a bit and see if I can come up with something.

@jonkerj
Copy link
Author

jonkerj commented Oct 1, 2022

  1. The CD doesn't currently build, it apparently just starts a debug shell (?) on the prepared system.

I did change put that in at some point, to investigate why/how things were going bad. Very hard (at least for me) to guess what was going wrong otherwise 😆

  1. full fails for all non-x86_64 architectures.

I had that working at some point, but it has been at least half a year since I touched that.

I must confess I have gotten less than motivated to finish this work, as it turns out it's very hard to reliably cross-build without a lot of DRY. It could be that it only needs some touching up, but with all these native dependencies it becomes messy/complex real quick.

@eladyn
Copy link
Member

eladyn commented Oct 1, 2022

Thanks for reporting back!

I did change put that in at some point, to investigate why/how things were going bad. Very hard (at least for me) to guess what was going wrong otherwise 😆

I assumed that too. 😁 Obviously nothing that should be merged, though.

  1. full fails for all non-x86_64 architectures.

I had that working at some point, but it has been at least half a year since I touched that.

Yeah, it seems that it is somehow a problem with the configuration of ubuntu-20.04 that github is using? With 18.04 and 22.04 it seems to build just fine.

I must confess I have gotten less than motivated to finish this work, as it turns out it's very hard to reliably cross-build without a lot of DRY. It could be that it only needs some touching up, but with all these native dependencies it becomes messy/complex real quick.

Totally understandable. If someone else wants to take up this PR (or I get around to it) I think we could merge it for the next release, otherwise I'd postpone that. Although the arm64 support seems certainly desirable and the simplifications that this PR brings are great!

@georgefst
Copy link

Totally understandable. If someone else wants to take up this PR (or I get around to it) I think we could merge it for the next release, otherwise I'd postpone that. Although the arm64 support seems certainly desirable and the simplifications that this PR brings are great!

I take it no one's interested then? I expect I'm not the only one out there who'd happily buy a few beers/coffees for anyone who gets this over the line - aarch64 is rapidly becoming a more popular architecture, especially now that it's effectively the default for RPis.

Otherwise, I'll try to find the time to look in to it myself, but I'm pretty swamped and this is not exactly my area of expertise.

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.

Provide arm binaries for all platforms.
7 participants