Skip to content

Extended CI suite #385

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Extended CI suite #385

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1662348
Prepare for new CI
LucasFA Mar 2, 2024
046d720
Add MSRV CI job
LucasFA Mar 2, 2024
6a37d30
Add OS_SPECIFIC_FEATURES to rust-ci job
LucasFA Mar 2, 2024
b605e17
Use spaces instead of commas as feature separator
LucasFA Mar 3, 2024
72a4946
Factor out Linux packages
LucasFA Mar 3, 2024
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
53 changes: 38 additions & 15 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,33 @@ env:
RUST_FEATURES: "rodio-backend,lyric-finder,media-control,image,notify,clipboard"
Copy link
Owner

@aome510 aome510 Mar 3, 2024

Choose a reason for hiding this comment

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

I realize once you surround the arguments with quotes, spaces can be used instead of commas. See my below suggestions

Copy link
Owner

@aome510 aome510 Mar 3, 2024

Choose a reason for hiding this comment

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

then you also need to update this variable to use spaces


jobs:
rust-ci:
msrv:
Copy link
Owner

@aome510 aome510 Mar 3, 2024

Choose a reason for hiding this comment

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

can msrv be put as a step of rust-ci instead to avoid code duplication between those two?

Copy link
Contributor Author

@LucasFA LucasFA Mar 5, 2024

Choose a reason for hiding this comment

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

I'm seeing if the solutions in this discussion to reuse matrices can be applied in a simple way here. Notable possible problem is that they don't use the same matrices as it is right now.
That's your main concern I assume, though the checkout and install packages steps could be abstracted away into another workflow, too.

Copy link
Owner

@aome510 aome510 Mar 6, 2024

Choose a reason for hiding this comment

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

I thought you only need to move

- name: Install cargo-hack
  uses: taiki-e/install-action@cargo-hack

- name: Run MSRV check 
  run: cargo hack check --rust-version --package spotify_player --all-targets --profile ci --features "${{ env.RUST_FEATURES }} ${{ matrix.OS_SPECIFIC_FEATURES }}"

to be in rust-ci.steps.

Copy link
Contributor Author

@LucasFA LucasFA Mar 6, 2024

Choose a reason for hiding this comment

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

I originally wanted to to have them as separate jobs in order to report these separately - at the end of the day, if it doesn't build in the current compiler that's a hard error, but breaking MSRV is salvageable, if anything by bumping the MSRV.

But we can do that, we would just have to check the log if it does break

Copy link
Owner

@aome510 aome510 Mar 6, 2024

Choose a reason for hiding this comment

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

yeah, one needs to check the action's logs if CI run fails anyway.

if: github.event.pull_request.draft != true
strategy:
fail-fast: false
Copy link
Owner

@aome510 aome510 Mar 3, 2024

Choose a reason for hiding this comment

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

why not fail-fast on this one?

Copy link
Contributor Author

@LucasFA LucasFA Mar 5, 2024

Choose a reason for hiding this comment

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

I think that being a check rather than build or test, the job should be fast enough to not matter for a few seconds, and adds the feedback on whether it broke for every platform or not - likely would be a platform dependant dependency.
Though if keeping the MSRV for builds with all (compatible) features it would have to be bumped anyway.

matrix:
os: [macOS-latest, windows-latest, ubuntu-latest]
include:
- os: ubuntu-latest
OS_SPECIFIC_FEATURES: ",daemon, sixel"
- os: macOS-latest
OS_SPECIFIC_FEATURES: ",sixel"
Comment on lines +23 to +26
Copy link
Owner

@aome510 aome510 Mar 3, 2024

Choose a reason for hiding this comment

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

Suggested change
- os: ubuntu-latest
OS_SPECIFIC_FEATURES: ",daemon, sixel"
- os: macOS-latest
OS_SPECIFIC_FEATURES: ",sixel"
- os: ubuntu-latest
OS_SPECIFIC_FEATURES: "daemon sixel"
- os: macOS-latest
OS_SPECIFIC_FEATURES: "sixel"


runs-on: ${{ matrix.os }}
steps:
- run: sudo apt-get update && sudo apt-get install libssl-dev libasound2-dev libdbus-1-dev libxcb-shape0-dev libxcb-xfixes0-dev
if: ${{ runner.os == 'Linux'}}
- uses: actions/checkout@v4

- uses: taiki-e/install-action@v2
with:
tool: cargo-hack

- uses: Swatinem/rust-cache@v2

- run: cargo hack check --rust-version --package spotify_player --all-targets --profile ci --features "${{ env.RUST_FEATURES }} ${{ matrix.OS_SPECIFIC_FEATURES }}"

rust-ci:
if: github.event.pull_request.draft != true

strategy:
Expand All @@ -22,6 +48,11 @@ jobs:
include:
- os: ubuntu-latest
toolchain: beta
OS_SPECIFIC_FEATURES: ",daemon, sixel"
- os: ubuntu-latest
OS_SPECIFIC_FEATURES: ",daemon, sixel"
- os: macOS-latest
OS_SPECIFIC_FEATURES: ",sixel"

runs-on: ${{ matrix.os }}

Expand All @@ -40,25 +71,17 @@ jobs:
toolchain: ${{ matrix.toolchain }}
components: clippy, rustfmt

- name: Cache cargo deps
uses: actions/cache@v3
with:
path: |
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
key: ${{ runner.os }}-rustc-${{ steps.install_toolchain.outputs.cachekey }}-cargo-${{ hashFiles('**/Cargo.lock') }}
restore-keys: ${{ runner.os }}-rustc-${{ steps.install_toolchain.outputs.cachekey }}
- uses: Swatinem/rust-cache@v2

- name: Cargo format
run: cargo fmt --all -- --check

- name: Cargo test
run: cargo test --no-default-features --features ${{ env.RUST_FEATURES }}
- name: Cargo clippy without features
run: cargo clippy --no-default-features --profile ci -- -D warnings

- name: Cargo clippy with all features
run: cargo clippy --no-default-features --features ${{ env.RUST_FEATURES }} -- -D warnings
run: cargo clippy --no-default-features --profile ci --features "${{ env.RUST_FEATURES }} ${{ matrix.OS_SPECIFIC_FEATURES }}" -- -D warnings

- name: Cargo clippy without features
run: cargo clippy --no-default-features -- -D warnings
- name: Cargo test
run: cargo test --no-default-features --profile ci --features "${{ env.RUST_FEATURES }} ${{ matrix.OS_SPECIFIC_FEATURES }}"

9 changes: 9 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,14 @@
members = ["spotify_player", "lyric_finder"]
resolver = "2"

[workspace.package]
rust-version = "1.74"

[profile.ci]
inherits = "dev"
debug = 0
strip = true
incremental = false

[profile.release]
debug = 1
1 change: 1 addition & 0 deletions lyric_finder/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
name = "lyric_finder"
version = "0.1.5"
edition = "2021"
rust-version.workspace = true
license = "MIT"
description = "A lyric finder library"
authors = ["Thang Pham <[email protected]>"]
Expand Down
8 changes: 6 additions & 2 deletions spotify_player/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "spotify_player"
version = "0.16.3"
authors = ["Thang Pham <[email protected]>"]
edition = "2021"
rust-version.workspace = true
license = "MIT"
description = "A Spotify player in the terminal with full feature parity"
repository = "https://github.com/aome510/spotify-player"
Expand Down Expand Up @@ -71,14 +72,17 @@ jackaudio-backend = ["streaming", "librespot-playback/jackaudio-backend"]
rodiojack-backend = ["streaming", "librespot-playback/rodiojack-backend"]
sdl-backend = ["streaming", "librespot-playback/sdl-backend"]
gstreamer-backend = ["streaming", "librespot-playback/gstreamer-backend"]

streaming = ["librespot-playback", "librespot-connect"]
lyric-finder = ["lyric_finder"]
media-control = ["souvlaki", "winit", "windows"]

image = ["viuer", "dep:image"]
sixel = ["image", "viuer/sixel"]
daemon = ["daemonize", "streaming"]

lyric-finder = ["lyric_finder"]
notify = ["notify-rust"]
clipboard = ["copypasta"]
daemon = ["daemonize", "streaming"]

default = ["rodio-backend", "media-control", "clipboard"]

Expand Down