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

Fix WIN32 CMake versioning #3300

Closed

Conversation

feynmanliang
Copy link

@feynmanliang feynmanliang commented Aug 20, 2020

Description

CMake build is erroring out on Windows because OBS_VERSION is not set:

Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.20190.
OBS_VERSION: 
CMake Error at CMakeLists.txt:85 (string):
  string sub-command REPLACE requires at least four arguments.


CMake Error at CMakeLists.txt:89 (list):
  list index: 1 out of range (-1, 0)


CMake Error at CMakeLists.txt:90 (list):
  list index: 2 out of range (-1, 0)

Motivation and Context

This defaults it to something sane to fix it.

How Has This Been Tested?

configure and generate from cmake-gui

Tested on CMake 3.18.2

Types of changes

Code cleanup (non-breaking change which makes code smaller or more readable)

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.

This change is Reviewable

@RytoEX
Copy link
Member

RytoEX commented Aug 20, 2020

The OBS_VERSION variable should already be set in cmake/Modules/ObsCpack.cmake line 25 for normal builds. Under what conditions are you encountering this error? This is the first time I've heard of this occurring.

@WizardCM
Copy link
Member

WizardCM commented Aug 20, 2020

This occurs when git is not in your path. I would vote for a more informative error. "Git not found in PATH, so the OBS version could not be set. Either fix this issue, or manually set OBS_VERSION_OVERRIDE"

@feynmanliang
Copy link
Author

feynmanliang commented Aug 20, 2020

Thanks for pointing me to that; this problem occurred because I cloned the repo using WSL's git but attempted to build using CMake in Windows. The .git directory is present (resulting the first conditional branch to execute), but the execute_process(... git describe...) returns empty because it is not on the Windows path.

I added message(STATUS "OBS_VERSION=${OBS_VERSION}") to confirm and got the output:

Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.20190.
OBS_VERSION=

I can confirm manually setting OBS_VERSION as a CMake variable does not fix the issue; it is overridden by the execute_process back to empty string.

Since it defaults to CPACK_PACKAGE_VERSION already in the case .git is not found, would we be okay with setting a sane default for OBS_VERSION when OBS_VERSION remains empty after attempting to set it by executing git? The format of CPACK_PACKAGE_VERSION is not quite what is expected in the WIN32 build (it's splitting on a "-" in https://github.com/obsproject/obs-studio/blob/master/CMakeLists.txt#L85 which is absent in CPACK_PACKAGE_VERSION), but I am happy to change the default to just append that from CPACK_PACKAGE_VERSION.

@WizardCM
Copy link
Member

Ah my bad, you need to manually set OBS_VERSION_OVERRIDE. I always forget about that one.

@feynmanliang
Copy link
Author

Yep that works; this could be as simple as a readme update then :) How does that sound?

@WizardCM
Copy link
Member

That's certainly an option, but I think having CMake report an accurate error would be handy too.

@feynmanliang
Copy link
Author

Will do both

@feynmanliang
Copy link
Author

feynmanliang commented Aug 26, 2020 via email

@WizardCM
Copy link
Member

WizardCM commented Oct 7, 2021

Closing this PR in favour of #4377 which was merged for 27.0 a while back.

@WizardCM WizardCM closed this Oct 7, 2021
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.

None yet

3 participants