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

cmake: Fix potential configure error when setting up buildnumber #122

Merged

Conversation

WarmUpTill
Copy link
Contributor

Description

If the CI environment variable is defined but GITHUB_RUN_ID is not this will result in a broken if statement.
This can apparently be the case for some Debian build pipelines.

This problem was originally reported here, while packaging obs-advanced-scene-switcher:
WarmUpTill/SceneSwitcher#1090

Motivation and Context

Fixes potential configure error.

How Has This Been Tested?

Tested by manually defining only the CI variable.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@RytoEX RytoEX requested a review from PatTheMav April 26, 2024 21:01
@PatTheMav
Copy link
Member

If it breaks because GITHUB_RUN_ID is not set, then the code (as it's currently implemented) works exactly as it should. Wrapping it in double quotes (to make it empty strings) defeats the purpose.

I think it would be better to first only check for the CI environment variable and then add a check for GITHUB_RUN_ID or GITLAB_RUN_ID and then use whatever is available to generate a build ID.

Because the code right now makes the assumption that CI would only ever be GitHub Actions, but I don't think it necessarily needs to be limited to that in the CMake files.

@WarmUpTill WarmUpTill force-pushed the debian-buildnumber-configure-issue branch from 1f0cb0e to 4826f4c Compare April 27, 2024 23:10
@WarmUpTill
Copy link
Contributor Author

Makes sense.
I have updated the PR.

@PatTheMav
Copy link
Member

Could you update the commit title to make it more explicit that this adds support for using GitLab run IDs as build numbers (because that's the precise scope of the change now)?

@WarmUpTill WarmUpTill force-pushed the debian-buildnumber-configure-issue branch from 4826f4c to 81a9667 Compare April 29, 2024 17:00
@WarmUpTill
Copy link
Contributor Author

Done :)

@PatTheMav PatTheMav merged commit e3688b7 into obsproject:master May 4, 2024
6 checks passed
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