Fix GH CI workflow #1810

erubboli · 2024-09-10T10:36:43Z

Fix Github CI workflow

This pull request implements comprehensive CI workflows for building and packaging releases across multiple platforms. The enhancements include:

Release Types

For each platform, we now produce two distinct release packages:

NodeGUI Package: Contains the Mintlayer Node GUI application.
Tools Package: Includes Mintlayer Node and command line utilities.

Platform-Specific Builds

macOS

NodeGUI release:
- Signed and packaged as a .dmg file for easy installation.
Tools package:
- Provided as a .zip archive.

Linux

Three package formats are now available:

.zip: For universal compatibility.
.deb: For Debian-based distributions (Ubuntu, Debian, etc.).
.rpm: For Red Hat-based distributions (Fedora, CentOS, etc.).

Windows

NodeGUI release:
- Packaged as a .zip file.
- Also available as an installer (.exe).
Tools package:
- Provided as a .zip archive.

Docker

Docker images are built and pushed to our repository.

Additional Notes

All builds are automated through our CI pipeline, ensuring consistency across releases.
Each artifact is accompanied by a SHA256 hash for verification purposes.
Release notes are automatically generated, listing all included files and their respective hashes.

ImplOfAnImpl · 2024-09-10T11:02:48Z

.github/workflows/build-docker.yml

@@ -0,0 +1,47 @@
+name: Build Docker


I'd suggest to put the release-related build-xxx files into a separate release subfolder.

like the idea but got this error from gh:

invalid value workflow reference: workflows must be defined at the top level of the .github/workflows/ directory

like the idea but got this error from gh

Then I'd suggest to rename the files at least, giving them the "release-" prefix, so that it's clear what those jobs are for.

ImplOfAnImpl · 2024-09-10T11:21:19Z

build-tools/osx/entitlements.plist

+    <!-- Debugging (remove for production) -->
+    <key>com.apple.security.get-task-allow</key>
+    <false/>


Should it be removed?

its set to false so its OK. when true it permits debugger

ImplOfAnImpl · 2024-09-10T11:27:24Z

build-tools/osx/sign_and_notarize.sh

@@ -0,0 +1,231 @@
+#!/bin/bash
+
+set -e


I'd also specify set -o nounset. Though it'll require refactoring the script a bit.

ImplOfAnImpl · 2024-09-10T11:32:44Z

build-tools/osx/sign_and_notarize.sh

+
+# Main execution
+main() {
+    if [ $# -eq 0 ]; then


azarovh · 2024-09-10T11:27:24Z

Cargo.toml

-installers = []
-# Target platforms to build apps for (Rust target-triple syntax)
-targets = ["aarch64-apple-darwin", "x86_64-apple-darwin", "x86_64-unknown-linux-gnu", "x86_64-pc-windows-msvc"]
-
 # Publish jobs to run in CI
 pr-run-mode = "plan"


Looks like this line was also related to dist and should be removed

azarovh · 2024-09-10T12:37:30Z

.github/workflows/build-linux.yml

+        for binary in "${BINARIES[@]}"; do
+          cp target/${{ matrix.arch }}-unknown-linux-gnu/release/$binary Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch }}/
+        done
+        zip -r Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch }}.zip Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch }}


btw why zip? Previously we used tar.xz and it looks more linux-friendly

ImplOfAnImpl · 2024-09-10T12:45:19Z

.github/workflows/build-linux.yml

+        cat << EOF > debian-gui/usr/share/applications/mintlayer-node-gui.desktop
+        [Desktop Entry]
+        Name=Mintlayer Node GUI
+        Exec=/usr/bin/node-gui


Btw,

I wonder whether we should rename the binary before installing it into the system. E.g. to mintlayer-node-gui. IMO node-gui sounds too generic and confusing without a context.

Also, why don't we provide a standalone version of node-gui anymore (i.e. just an executable in an archive) in addition to the deb/rpm? Some people (myself including) would prefer a standalone app.

IMO node-gui sounds too generic and confusing without a context.

Same for the deb/rpm that install command-line apps.

I agree with you; binaries should have a prefix to avoid conflicts when installed system-wide. However, implementing this change would break any existing scripts. It's a delicate matter, and I'm not sure we can make this change easily now.

I agree with you; binaries should have a prefix to avoid conflicts when installed system-wide. However, implementing this change would break any existing scripts. It's a delicate matter, and I'm not sure we can make this change easily now.

I wonder whether we should publish the deb/rpm at all in such a case. At least for the command-line tools. Because not only those binaries have generic names, their --help output doesn't even mention they are related to Mintlayer. So users may not have any idea what has just been installed into their system.

ImplOfAnImpl · 2024-09-10T12:51:50Z

.github/workflows/build-linux.yml

+          cp target/${{ matrix.arch }}-unknown-linux-gnu/release/$binary Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch }}/
+        done


The zip archive and packages have inconsistent naming for arm64 - the packages have the suffix "arm64" and the archive "aarch64". Not a big deal, but can be confusing for users.

The zip archive and packages have inconsistent naming for arm64 - the packages have the suffix "arm64" and the archive "aarch64". Not a big deal, but can be confusing for users.

Same thing for x86_64 actually (the suffixes are x86_64 and amd64)

azarovh · 2024-09-10T13:06:29Z

.github/workflows/release.yml

-#
-# Note that the Github Release will be created with a generated
-# title/body based on your changelogs.
+name: Build, Sign, and Release


This is now builds and signs artefacts for every PR. It should be limited to release branches I guess

ImplOfAnImpl · 2024-09-10T18:02:49Z

.github/workflows/build-linux.yml

+
+jobs:
+  build:
+    runs-on: ubuntu-latest


I don't think we should use the latest image here, because it effectively determines the minimum version of glibc required by the binaries. E.g. I've checked your test binaries - according to objdump, node-daemon needs at least one symbol from glibc v2.34, while node-gui needs a symbol from v2.35.
Glibc v2.35 is the one used by Ubuntu 22.04, which is what "ubuntu-latest" currently means on GitHub Actions. But they'll update to 24.04 eventually and our next release may suddenly require even a newer version of a Linux distro.
For comparison, Debian 11 (which, as I've googled, will be supported until 08.2026), has glibc v2.31, so the binaries won't work there.
And RHEL 8 (supported until 2029) has glibc v2.28.

So at the very least I'd suggest to specify a concrete Linux distro here.
GitHub actions should still support ubuntu-20.04 (which uses glibc v2.31), we can use this one, so that the binaries work on Debian 11 at least.

An alternative solution might be to use one of the "manylinux" docker images, which are used to compile Python extensions. I've found this article where the author has successfully used one of the manylinux images with GitHub actions.

Or we may want to switch to using musl-libc targets, so that the binaries don't depend on glibc at all. Though those targets are still considered "Tier 2" so I'm not sure if it's a good idea (in any case, it's not a good idea to switch to musl in the current release).

P.S. We could also provide musl-based binaries in addition to glibc-based ones, so that people on older systems could have an alternative.

fix workflow

7416207

erubboli requested review from azarovh and ImplOfAnImpl as code owners September 10, 2024 10:36

ImplOfAnImpl approved these changes Sep 10, 2024

View reviewed changes

azarovh approved these changes Sep 10, 2024

View reviewed changes

azarovh reviewed Sep 10, 2024

View reviewed changes

ImplOfAnImpl reviewed Sep 10, 2024

View reviewed changes

azarovh reviewed Sep 10, 2024

View reviewed changes

ImplOfAnImpl reviewed Sep 10, 2024

View reviewed changes

erubboli added 11 commits September 11, 2024 13:58

implement suggestions

b5e9931

clean up cargo dist

fd048a3

set nounset in bash script

4038849

update Cargo.lock

ed300c7

remove trailing spaces

fc7cae3

add license to build.rs

65ed77b

dont use unwrap

b85a086

fix checks

8da2292

missed std::fs

27eb16c

move uses inside the function

c7f24d6

use is_err()

d11cfad

erubboli merged commit 301bdde into master Sep 11, 2024
18 checks passed

erubboli deleted the fix_workflow branch September 11, 2024 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH CI workflow #1810

Fix GH CI workflow #1810

erubboli commented Sep 10, 2024

ImplOfAnImpl Sep 10, 2024

erubboli Sep 10, 2024

ImplOfAnImpl Sep 11, 2024

ImplOfAnImpl Sep 10, 2024

erubboli Sep 10, 2024

ImplOfAnImpl Sep 10, 2024

ImplOfAnImpl Sep 10, 2024

azarovh Sep 10, 2024

azarovh Sep 10, 2024

ImplOfAnImpl Sep 10, 2024

ImplOfAnImpl Sep 10, 2024

erubboli Sep 10, 2024

ImplOfAnImpl Sep 11, 2024

ImplOfAnImpl Sep 10, 2024

ImplOfAnImpl Sep 10, 2024

azarovh Sep 10, 2024

ImplOfAnImpl Sep 10, 2024

		cp target/${{ matrix.arch }}-unknown-linux-gnu/release/$binary Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch }}/
		done

Fix GH CI workflow #1810

Fix GH CI workflow #1810

Conversation

erubboli commented Sep 10, 2024

Fix Github CI workflow

Release Types

Platform-Specific Builds

macOS

Linux

Windows

Docker

Additional Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment