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.

Sign up for GitHub

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

ci: add simple build test workflow #696

Merged
merged 4 commits into from
Jan 19, 2025
Merged

ci: add simple build test workflow #696

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4687769
ci: add simple build test workflow
MichaIng Jan 13, 2025
a934578
build: do not query GitHub API if origin is upstream
MichaIng Jan 16, 2025
115fed0
Revert "build: do not query GitHub API if origin is upstream"
MichaIng Jan 19, 2025
ce05e39
build: add warning and error indication to messages
MichaIng Jan 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/dependabot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
version: 2
updates:
- package-ecosystem: "github-actions"
directory: "/"
schedule:
interval: "daily"
target-branch: "master"
labels:
- "enhancement"
19 changes: 19 additions & 0 deletions .github/workflows/build-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
name: Build test
on: [pull_request, push, workflow_dispatch]
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
permissions:
contents: read
jobs:
build:
strategy:
matrix:
arch: [armhf, arm64, amd64, riscv64]
fail-fast: false
name: Build on ${{ matrix.arch }}
if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.owner.login != github.event.pull_request.base.repo.owner.login
kimtore marked this conversation as resolved.
Show resolved Hide resolved
runs-on: ubuntu-24.04
steps:
- uses: actions/checkout@v4
- run: make ${{ matrix.arch }}
21 changes: 15 additions & 6 deletions build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@ packages() {

cd librespot

LIBRESPOT_VER="$(git describe --tags "$(git rev-list --tags --max-count=1)" 2>/dev/null || echo unknown)"
LIBRESPOT_HASH="$(git rev-parse HEAD | cut -c 1-7 2>/dev/null || echo unknown)"
echo 'Obtaining latest Librespot Git repository tag for DEB package version suffix ...'
LIBRESPOT_VER="$(git describe --tags "$(git rev-list --tags --max-count=1)" || echo unknown)"
LIBRESPOT_HASH="$(git rev-parse HEAD | cut -c 1-7 || echo unknown)"

echo "Build Librespot binary..."
cargo build --jobs "$(nproc)" --profile release --target "$BUILD_TARGET" --no-default-features --features "alsa-backend pulseaudio-backend with-avahi"
Expand Down Expand Up @@ -156,9 +157,17 @@ START_BUILDS=$(now)

cd /mnt/raspotify

# Get the git rev of raspotify for .deb versioning
RASPOTIFY_GIT_VER="$(git describe --tags "$(git rev-list --tags --max-count=1)" 2>/dev/null || echo unknown)"
RASPOTIFY_HASH="$(git rev-parse HEAD | cut -c 1-7 2>/dev/null || echo unknown)"
echo 'Obtaining latest Git repository tag for DEB package version ...'
RASPOTIFY_GIT_VER="$(git describe --tags "$(git rev-list --tags --max-count=1)" || :)"
if [ -z "$RASPOTIFY_GIT_VER" ]; then
echo 'Could not obtain latest tag from local repository. Obtaining it from upstream: https://api.github.com/repos/dtcooper/raspotify/tags'
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about starting the whole subroutine with git fetch --tags origin instead? This will ensure tags locally and not trigger a HTTP request to GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is not that tags are locally missing, but that the fork repo in fact does not have any tags, also not online. I could rephrase the log to "local/origin", to make that clear. It just makes sense for anyone who contributes to the project, forking the repo to make an edit, then open a PR upstream: forks do not contain any tags, unless they are created at the fork.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. But is it really necessary to have the correct tag output in the build unless it is a release build? What is the value of having this tag?

Copy link
Contributor Author

@MichaIng MichaIng Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The tags are used for the version string or the DEB packages. Originally, without a tag, "unknown" is used, which is no valid version for DEB, as it strictly requires version strings to start with an integer. Of course we could "echo 1" or so, but I see no reason for DEB packages built on forks to have no proper version string, which compares well to upstream/distro/installed versions of the package, even if it is used for testing purposes only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very well, however I fear it will potentially silently ignore tags from a (faulty) local copy.
I want the checked out copy to be the source of truth for the version tag, not the upstream.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi again.
How about a guard before L163 that checks whether the locally checked-out copy's origin is dtcooper/raspotify? This way we can have the best of both worlds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll implement this in a few hours.

Copy link
Contributor Author

@MichaIng MichaIng Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not so trivial to know whether the local repo is supposed to be dtcooper/raspotify or not. Git itself has no concept for owner or repo name, this is all a GitHub-only thing. Best I could find is checking the origin remote URL, which is by default matching the repository that was originally cloned. GitHub allows some variations for this URL, and even that they are all redirected to a canonical, git remote still returns the variant that was entered when cloning the repo.

... okay and it does not work for the pull request triggered runs: On my fork, the push triggered builds were successful, but here not. Reason is that the origin indeed is https://github.com/dtcooper/raspotify, and the "branch"/"ref" reflects the code from my fork instead. So we would need to check the ref as well, whether it is a pull request (should be possible to see from the name scheme). But this is becoming quite complicated and error-prone/not failsafe, worth the hassle?

EDIT: If this requires some more thoughts, we can leave the build script untouched for now, and only replace the "unknown" with e.g. a static "0.1" from within the workflow, for tests to succeed as well on forks. In the workflow we can derive from variables, who the repo/head owner is, hence whether we can count on tags or not. I mean this PR was originally about adding a build test, not about making the build script itself compatible with forks. That can be done any time later, if there is a demand.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But this is becoming quite complicated and error-prone/not failsafe, worth the hassle?

Screw it then -- it was worth a shot, but I prefer simplicity.

I'll try merging this PR as-is and see where that gets us :)

In the future I want to implement nightly builds, i.e. the latest commit on main should produce a GitHub pre-release where users always can download the latest debs, for testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll merge through the GitHub clickops after you revert that last commit then.

RASPOTIFY_GIT_VER="$(curl -sSf "https://api.github.com/repos/dtcooper/raspotify/tags" | awk -F\" '/^ *"name": "/{print $4;exit}' || :)"
fi
if [ -z "$RASPOTIFY_GIT_VER" ]; then
echo 'Could not obtain latest tag from upstream repository either. Exiting ...'
exit 1
fi
RASPOTIFY_HASH="$(git rev-parse HEAD | cut -c 1-7 || echo unknown)"

echo "Build Raspotify $RASPOTIFY_GIT_VER~$RASPOTIFY_HASH..."

Expand All @@ -181,7 +190,7 @@ case $ARCHITECTURE in
esac

# Fix broken permissions resulting from running the Docker container as root.
[ $(id -u) -eq 0 ] && chown -R "$PERMFIX_UID:$PERMFIX_GID" /mnt/raspotify 2>/dev/null
[ $(id -u) -eq 0 ] && chown -R "$PERMFIX_UID:$PERMFIX_GID" /mnt/raspotify

BUILD_TIME=$(duration_since "$START_BUILDS")

Expand Down