Skip to content

build(ci): Add missing '+ffmpeg' label to build job and steps - #3251

Open
tsunamistate wants to merge 1 commit into
TheSuperHackers:mainfrom
tsunamistate:add-ffmpeg-label
Open

build(ci): Add missing '+ffmpeg' label to build job and steps#3251
tsunamistate wants to merge 1 commit into
TheSuperHackers:mainfrom
tsunamistate:add-ffmpeg-label

Conversation

@tsunamistate

@tsunamistate tsunamistate commented Sep 3, 2026

Copy link
Copy Markdown

Follow up to #3184

Due to me to not Reading the Freaking Manual enough, +ffmpeg label is only added to “Configure” step for the job that has it enabled (GeneralsMD win32-vcpkg)

26-09-03-t_14-51-12

Add it everywhere else (job name and other steps) so that it's clear what build has FFmpeg support enabled

No functional changes

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Show FFmpeg support in vcpkg workflow labels

⚙️ Configuration changes 🕐 Less than 5 minutes

Grey Divider

AI Description

• Appends +ffmpeg to the toolchain job name when FFmpeg is enabled.
• Identifies FFmpeg-enabled builds in CMake build and artifact upload step labels.
High-Level Assessment

The conditional label suffix matches the workflow's existing tools and extras naming convention and is the most direct approach. Centralizing the generated label was considered but would add unnecessary complexity for three display-only references.

Files changed (1) +3 / -3

Other (1) +3 / -3
build-toolchain.ymlLabel FFmpeg-enabled workflow jobs and steps +3/-3

Label FFmpeg-enabled workflow jobs and steps

• Appends a conditional '+ffmpeg' suffix to the build job, CMake build step, and artifact upload step display names. This improves workflow visibility without changing build execution or uploaded artifact names.

.github/workflows/build-toolchain.yml

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Informational

1. Collect step omits FFmpeg 🐞 Bug ≡ Correctness
Description
The artifact collection step still omits the +ffmpeg suffix, so the FFmpeg-enabled build remains
indistinguishable at that step despite the PR's labeling goal. This creates inconsistent labels
within the same job.
Code

.github/workflows/build-toolchain.yml[181]

+      - name: Upload ${{ inputs.game }} ${{ inputs.preset }}${{ inputs.tools && '+t' || '' }}${{ inputs.extras && '+e' || '' }}${{ inputs.ffmpeg && '+ffmpeg' || '' }} Artifact
Evidence
The workflow passes ffmpeg: true for the win32-vcpkg build, and the configure, build, and upload
step names include the conditional suffix. The collection step at line 164 uses the same
preset/tools/extras label pattern but is the sole artifact lifecycle step without the FFmpeg suffix.

.github/workflows/ci.yml[149-166]
.github/workflows/build-toolchain.yml[141-164]
.github/workflows/build-toolchain.yml[181-185]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The FFmpeg-enabled workflow labels the configure, build, and upload steps with `+ffmpeg`, but not the artifact collection step.

## Issue Context
Use the same conditional `${{ inputs.ffmpeg && '+ffmpeg' || '' }}` suffix already applied to the surrounding step names.

## Fix Focus Areas
- .github/workflows/build-toolchain.yml[164-164]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread .github/workflows/build-toolchain.yml
@greptile-apps

greptile-apps Bot commented Sep 3, 2026

Copy link
Copy Markdown

Greptile Summary

This PR makes FFmpeg-enabled vcpkg builds easier to identify in GitHub Actions without changing build or artifact behavior.

  • Appends +ffmpeg to the reusable build job name when FFmpeg is enabled.
  • Adds the same conditional label to the build, collection, and upload step names.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
.github/workflows/build-toolchain.yml Adds consistent conditional +ffmpeg text to workflow display names while leaving commands and artifact identifiers unchanged.

Reviews (2): Last reviewed commit: "Add '+ffmpeg' label to build job and ste..." | Re-trigger Greptile

@tsunamistate tsunamistate changed the title build(vcpkg): Add missing '+ffmpeg' label to build job and steps build(ci): Add missing '+ffmpeg' label to build job and steps Sep 3, 2026
@xezon

xezon commented Sep 3, 2026

Copy link
Copy Markdown

Do we need this at all in the title? What is it good for?

@tsunamistate

Copy link
Copy Markdown
Author

So that CI jobs display the +ffmpeg label when it's enabled
In my previous PR, GeneralsMD win32-vcpkg has FFmpeg support enabled, but it's only shown in one step (Configure)

26-09-03-t_14-51-12

Now it shows in both the job name and all steps, so it's clear that this has FFmpeg support

26-09-03-t_15-59-37

Why do you need to know which build has FFmpeg support? For now, support is incomplete, FFmpeg build has no audio is cutscenes, but it's useful to have a build for testing.

@xezon

xezon commented Sep 3, 2026

Copy link
Copy Markdown

Do we need this extra keyword in the target name? Can we not imply it with vcpkg, that it always has ffmpeg enabled? Why do we build only one of the vcpkg targets with ffmpeg? Should we not build all of them with it?

@tsunamistate

Copy link
Copy Markdown
Author

Can we not imply it with vcpkg, that it always has ffmpeg enabled?

I just made FFmpeg optional and disabled by default in 3184, so vcpkg does NOT imply enabled FFmpeg support

Why do we build only one of the vcpkg targets with ffmpeg?

@bobtista sorry to bother you, but can you please explain why you asked to enable FFmpeg on only one job?
I would assume it's because, a) FFmpeg support is incomplete and b) FFmpeg compilation takes a long time without caching.

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.

2 participants