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 build directory exclusion #11501

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

RytoEX
Copy link
Member

@RytoEX RytoEX commented Nov 8, 2024

Description

The regex was incorrectly excluding any file with build in the name. The intent was to exclude any build directories, so we should be able to restrict this.

Motivation and Context

Fixes #11443.

How Has This Been Tested?

It hasn't.

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 added Bug Fix Non-breaking change which fixes an issue Linux Affects Linux labels Nov 8, 2024
@RytoEX RytoEX added this to the OBS Studio 31 milestone Nov 8, 2024
@RytoEX RytoEX requested a review from PatTheMav November 8, 2024 20:42
@RytoEX RytoEX self-assigned this Nov 8, 2024
@PatTheMav
Copy link
Member

I'm not sure if the "proper" fix is to remove the tarball entirely - given that only one user has reported that defect (and didn't even notice the entire extent of it), it seems like we already spent too much maintenance effort on a very niche requirement.

@tgurr
Copy link
Contributor

tgurr commented Nov 13, 2024

I'm not sure if the "proper" fix is to remove the tarball entirely - given that only one user has reported that defect (and didn't even notice the entire extent of it), it seems like we already spent too much maintenance effort on a very niche requirement.

@PatTheMav The tarball including the whole source is required as it includes submodules (like obs-browser/obs-websocket) which would be missing otherwise. The reason why you're not seeing others complaining is probably because no proper distribution actually bothers with trying to package beta versions, you'll likely see these pop up when a proper release has been done with the things being broken. As I/we were hit by issues previously I got cautious in regards to obs and try to temporarily package beta versions as well so we can try to catch these issues early e.g. before a proper release has been done.

@RytoEX
Copy link
Member Author

RytoEX commented Nov 13, 2024

Pushed a change that should no longer exclude /buildspec.json or the other files. Do note that it will still exclude /build-aux/, but the current regex was already doing that.

This should exclude:

./build/
./build_x64/
./build_x86/
./build_x86_64/
./build_arm64/
./build_someotherarch/

This should include:

./buildspec.json
./cmake/common/buildnumber.cmake
./cmake/common/buildspec_common.cmake

@RytoEX
Copy link
Member Author

RytoEX commented Nov 14, 2024

After reviewing .github/scripts/.package.zsh, it seems like the only currently valid build directory on CI for the purpose of generating a source package via CPack is build_x86_64.

local -r -a _valid_targets=(
macos-x86_64
macos-arm64
ubuntu-x86_64
)

${cmake_bin} --build build_${target%%-*} --config ${config} --target package_source ${cmake_args}
output_name="${output_name}-sources"

We could just do this instead:

set(CPACK_SOURCE_IGNORE_FILES "/.git" "/build_x86_64" "/.ccache" "/.deps")

Perhaps it's not as elegant, but it would resolve the issue.

@PatTheMav
Copy link
Member

After reviewing .github/scripts/.package.zsh, it seems like the only currently valid build directory on CI for the purpose of generating a source package via CPack is build_x86_64.

local -r -a _valid_targets=(
macos-x86_64
macos-arm64
ubuntu-x86_64
)

${cmake_bin} --build build_${target%%-*} --config ${config} --target package_source ${cmake_args}
output_name="${output_name}-sources"

We could just do this instead:

set(CPACK_SOURCE_IGNORE_FILES "/.git" "/build_x86_64" "/.ccache" "/.deps")

Perhaps it's not as elegant, but it would resolve the issue.

The _x64_64 token is dynamic, however you could probably use CMAKE_SYSTEM_PROCESSOR to match it, so /build_${CMAKE_SYSTEM_PROCESSOR} should give you the exact same name dynamically (and thus matches the code/behaviour used by the script to define the build directory).

@RytoEX
Copy link
Member Author

RytoEX commented Nov 15, 2024

The _x64_64 token is dynamic

In that specific part of the script, it isn't dynamic per se. The .package.zsh script bails out if the target is not in _valid_targets, so the part of the script that creates the sources package on the Ubuntu host can only have x86_64 as the arch at this time. This part of the script only runs on Ubuntu and not on macOS.

That said, /build_${CMAKE_SYSTEM_PROCESSOR} is probably fine, since in this case on Ubuntu it should return x86_64.

The regex was incorrectly excluding any file with build in the name. The
intent was to exclude any build directories, so we should be able to
restrict this.
@RytoEX RytoEX merged commit b5f4958 into obsproject:master Nov 15, 2024
14 checks passed
@RytoEX RytoEX deleted the fix-cmake-linux-cpackconfig branch November 15, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Linux Affects Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[31.0.0-beta2] - source tarball is missing cmake/common/{buildnumber.cmake,buildspec_common.cmake}
3 participants