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 for real #11543

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RytoEX
Copy link
Member

@RytoEX RytoEX commented Nov 21, 2024

Description

Upon further review, the build directory is never set to build_$arch. Currently, our CMake Presets on Ubuntu only use build_ubuntu. However, we can attempt to be flexible here and simply exclude the build directory configured in CMake.

Motivation and Context

Want this to really be fixed. Follow-up to #11501.

Discovered after tagging 31.0.0-rc1 that the build directory was not excluded from the source package. Dug a bit deeper and realized that the build directory on CI is always build_ubuntu and never build_$arch//build_${CMAKE_SYSTEM_PROCESSOR}.

We could also simply say "we only support our provided presets" and hardcode this to "build_ubuntu" as we do in CMakePresets.json, but this should use that same value.

How Has This Been Tested?

Tested locally on WSL.

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.

Upon further review, the build directory is never set to build_$arch.
Currently, our CMake Presets on Ubuntu only use build_ubuntu. However,
we can attempt to be flexible here and simply exclude the build
directory configured in CMake.
@RytoEX RytoEX added Bug Fix Non-breaking change which fixes an issue CI labels Nov 21, 2024
@RytoEX RytoEX added this to the OBS Studio 31 milestone Nov 21, 2024
@RytoEX RytoEX self-assigned this Nov 21, 2024
@@ -16,7 +16,7 @@ endif()

set(CPACK_SOURCE_PACKAGE_FILE_NAME "obs-studio-${CPACK_PACKAGE_VERSION}-sources")
set(CPACK_SOURCE_GENERATOR "TGZ")
set(CPACK_SOURCE_IGNORE_FILES "/.git" "/build_${CMAKE_SYSTEM_PROCESSOR}" "/.ccache" "/.deps")
set(CPACK_SOURCE_IGNORE_FILES "/.git" "${CMAKE_BINARY_DIR}" "/.ccache" "/.deps")
Copy link
Member

Choose a reason for hiding this comment

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

CMAKE_BINARY_DIR should be an absolute path - I'm surprised that it will match a local relative path.

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC, CMake converts these paths to absolute paths if they are relative, but ultimately uses absolute paths. I'll go re-read the source later to check, but it worked on my machine.

Copy link
Member Author

@RytoEX RytoEX Nov 21, 2024

Choose a reason for hiding this comment

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

The Ubuntu presets set binaryDir to "${sourceDir}/build_ubuntu". Let's say CMAKE_BINARY_DIR is /home/runner/work/obs-studio/obs-studio/build_ubuntu.

CMake sets CPACK_SOURCE_IGNORE_FILES to CPACK_IGNORE_FILES.
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Modules/CPack.cmake#L984

The value of CPACK_IGNORE_FILES is assigned to cpackIgnoreFiles.
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/CPack/cmCPackGenerator.cxx#L344

cpackIgnoreFiles is used to build ignoreFilesRegex:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/CPack/cmCPackGenerator.cxx#L343-352

The top directory logged here is an absolute path.
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/CPack/cmCPackGenerator.cxx#L373-378

 CPack: - Install directory: /home/runner/work/obs-studio/obs-studio

The top directory is used to make findExpr:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/CPack/cmCPackGenerator.cxx#L376-382

findExpr = /home/runner/work/obs-studio/obs-studio/*

findExpr is passed to cmsys::Glob::FindFiles(const std::string& inexpr, GlobMessages* messages) which assigns inexpr to expr and changes expr to an absolute path if it was not already one:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L353-356
In this case, both expr and inexpr would be /home/runner/work/obs-studio/obs-studio/*

If the last slash in expr is not the first character, last_slash is set to that position value. If last_slash is greater than 0, skip is set to that same value:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L359-374
In this case, last_slash and skip should be 39.

Since skip is greater than 0, expr is trimmed to erase all directories and only leave the last path component:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L397-399
In this case, expr becomes /*.

Then, cexpr becomes * and is added to the Expressions vector:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L401-414

Since skip is greater than zero, this branch is taken:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L417-419

this->ProcessDirectory(0, "/home/runner/work/obs-studio/obs-studio/", messages);

In ProcessDirectory(), the first path is taken because start is 0, and the Expressions vector is size 1, so 0 = 1-1:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L287-293

In RecurseDirectory(), the directory /home/runner/work/obs-studio/obs-studio/ is loaded, and all files in it are recursively added to the Files vector. The file paths are constructed as absolute paths:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L205-209

In cmsys::Glob, Relative is initialized to "":
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L51

None of cmsys::Glob::FindFiles, anything in its calls, or the setup in cmCPackGenerator::InstallProjectViaInstalledDirectories changes the Relative member variable. That being the case, when files are added to cmsys::Glob's this->Internal->Files with AddFile(), they are using absolute paths:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L447-454

These files as absolute paths are retrieved using cmsys::Glob::GetFiles():
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/CPack/cmCPackGenerator.cxx#L388
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/kwsys/Glob.cxx#L68-71

After recursively globbing all files in the source directory, files are checked to see if they should be ignored.
Directory paths are normalized to end with a slash:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/CPack/cmCPackGenerator.cxx#L388-395

Paths that match any regex in ignoreFilesRegex (see above) are ignored:
https://gitlab.kitware.com/cmake/cmake/-/blob/v3.31.1/Source/CPack/cmCPackGenerator.cxx#L396-405

This does also mean that if any part of the path preceding the source directory (parent directories) matches /.git, /.ccache or /.deps, then the entire obs-studio source folder will be ignored.

I stepped through most of FindFiles() and ProcessDirectory() with Compiler Explorer to confirm their behavior.

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 CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants