Replace platform-specific packaging scripts with CPack and add resource management helper#274
Replace platform-specific packaging scripts with CPack and add resource management helper#274
Conversation
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
|
@copilot Can you add a function that simplifies the systems that add resources to the bundle or build directory (lines 258 to 279 of CMakeLists.txt)? There could be a function |
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
Added The function handles:
Usage example: set_target_resources(Partiels FILE "${SOURCE}/file.txt" DESTINATION Templates)
set_target_resources(Partiels DIRECTORY "${SOURCE}/Preset/" DESTINATION Preset)This reduced CMakeLists.txt by ~15 lines and eliminates duplication between build and install rules. |
|
The code can still be simplified with: if(WIN32)
set_target_resources(Partiels FILE ${PARTIELS_BINARYDATA_DIRECTORY}/Resource/generate-translation-file.ps1 DESTINATION Scripts)
else()
set_target_resources(Partiels FILE ${PARTIELS_BINARYDATA_DIRECTORY}/Resource/generate-translation-file.sh DESTINATION Scripts)
endif()and |
Co-authored-by: pierreguillot <1409918+pierreguillot@users.noreply.github.com>
Implemented in commit 458939e. Changes:
Example: # Translation script (platform-specific)
if(WIN32)
set_target_resources(Partiels FILE ${BINARY_DIR}/script.ps1 DESTINATION Scripts)
else()
set_target_resources(Partiels FILE ${BINARY_DIR}/script.sh DESTINATION Scripts)
endif()
# Plugin
set_target_resources(Partiels PLUGIN partiels-vamp-plugins)This reduced CMakeLists.txt by another 10+ lines and eliminated all manual plugin copying/installation code. |
There was a problem hiding this comment.
Pull request overview
This PR successfully modernizes the packaging system by replacing 80+ lines of platform-specific CMake code with a unified CPack-based solution. The changes eliminate InnoSetup dependency on Windows in favor of WiX MSI generation, consolidate Linux packaging with proper TGZ/DEB/RPM support, and introduce a reusable set_target_resources() helper function that dramatically simplifies resource management across all platforms.
Key improvements:
- Unified packaging configuration in CPack.cmake eliminates duplicate code and reduces maintenance burden
- New
set_target_resources()function provides single-point resource management for files, directories, and plugins - CI workflow updated to use
cpackcommands instead of custom scripts
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| CPack.cmake | New unified packaging configuration supporting WiX/NSIS (Windows), TGZ/DEB/RPM (Linux), and DMG (macOS) with set_target_resources() helper function |
| CMakeLists.txt | Simplified from ~105 lines to ~17 lines by replacing custom packaging logic with include(CPack.cmake) and function calls |
| BinaryData/Resource/windows-package.iss | Removed InnoSetup configuration file (replaced by WiX/NSIS CPack generators) |
| .gitignore | Added build_test directory exclusion |
| .github/workflows/ci.yml | Updated CI to use cpack commands, replaced InnoSetup with WiX toolset, changed artifact from .exe to .msi |
| install(DIRECTORY "${source_path}" DESTINATION ${dest_dir} COMPONENT Runtime) | ||
| endif() | ||
| elseif(UNIX AND NOT APPLE) | ||
| if(resource_type STREQUAL "FILE") | ||
| install(FILES "${source_path}" DESTINATION opt/Partiels/${dest_dir} COMPONENT Runtime) | ||
| elseif(resource_type STREQUAL "DIRECTORY") | ||
| install(DIRECTORY "${source_path}" DESTINATION opt/Partiels/${dest_dir} COMPONENT Runtime) |
There was a problem hiding this comment.
The install(DIRECTORY ...) command behavior is inconsistent with expectations. When source_path has a trailing slash (e.g., "${PARTIELS_BINARYDATA_DIRECTORY}/Preset/"), CMake copies the contents of the directory, not the directory itself. Without a trailing slash, it copies the directory. This means:
- Line 240 in CMakeLists.txt:
DIRECTORY "${PARTIELS_BINARYDATA_DIRECTORY}/Preset/"will install contents directly toopt/Partiels/Preset/(correct) - Line 241:
DIRECTORY "${PARTIELS_BINARYDATA_DIRECTORY}/Translations/"will install contents directly toopt/Partiels/Translations/(correct)
However, this creates ambiguity. Consider documenting this behavior or normalizing the paths to ensure consistent behavior. The same issue exists for Windows on line 110.
| install(DIRECTORY "${source_path}" DESTINATION ${dest_dir} COMPONENT Runtime) | |
| endif() | |
| elseif(UNIX AND NOT APPLE) | |
| if(resource_type STREQUAL "FILE") | |
| install(FILES "${source_path}" DESTINATION opt/Partiels/${dest_dir} COMPONENT Runtime) | |
| elseif(resource_type STREQUAL "DIRECTORY") | |
| install(DIRECTORY "${source_path}" DESTINATION opt/Partiels/${dest_dir} COMPONENT Runtime) | |
| # Normalize source_path to remove trailing slash for consistent install behavior | |
| string(REGEX REPLACE "/$" "" normalized_source_path "${source_path}") | |
| install(DIRECTORY "${normalized_source_path}" DESTINATION ${dest_dir} COMPONENT Runtime) | |
| endif() | |
| elseif(UNIX AND NOT APPLE) | |
| if(resource_type STREQUAL "FILE") | |
| install(FILES "${source_path}" DESTINATION opt/Partiels/${dest_dir} COMPONENT Runtime) | |
| elseif(resource_type STREQUAL "DIRECTORY") | |
| # Normalize source_path to remove trailing slash for consistent install behavior | |
| string(REGEX REPLACE "/$" "" normalized_source_path "${source_path}") | |
| install(DIRECTORY "${normalized_source_path}" DESTINATION opt/Partiels/${dest_dir} COMPONENT Runtime) |
| set(CPACK_NSIS_MUI_ICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.png") | ||
| set(CPACK_NSIS_MUI_UNIICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.png") |
There was a problem hiding this comment.
NSIS requires icons in .ico format, not .png. The CPACK_NSIS_MUI_ICON and CPACK_NSIS_MUI_UNIICON settings with PNG files will cause errors. Convert icon_square.png to .ico format or use an existing .ico file.
| set(CPACK_NSIS_MUI_ICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.png") | |
| set(CPACK_NSIS_MUI_UNIICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.png") | |
| set(CPACK_NSIS_MUI_ICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.ico") | |
| set(CPACK_NSIS_MUI_UNIICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.ico") |
| set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_SOURCE_DIR}/LICENSE") | ||
| set(CPACK_RESOURCE_FILE_README "${CMAKE_CURRENT_BINARY_DIR}/Install.txt") | ||
|
|
||
| # Only install runtime components, skip development files |
There was a problem hiding this comment.
The comment "Only install runtime components, skip development files" is helpful, but it's unclear what development files are being filtered. Consider expanding the comment to clarify what this achieves:
# Only install runtime components, skip development files (lib/, include/, CMake config files)
# The fourth parameter filters by component name; "/" means install at the project root| # Only install runtime components, skip development files | |
| # Only install runtime components in the package; skip development files such as libraries (lib/), headers (include/), and CMake config files. |
| install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/ChangeLog.txt" DESTINATION . COMPONENT Runtime) | ||
|
|
||
| # Code signing support (if enabled) | ||
| if(PARTIELS_NOTARIZE AND PARTIELS_CODESIGN_WINDOWS_KEYFILE) |
There was a problem hiding this comment.
The variable PARTIELS_NOTARIZE is used in the condition but is never defined in the codebase. This will always evaluate to false, effectively disabling Windows code signing. Consider either:
- Adding
set(PARTIELS_NOTARIZE OFF CACHE BOOL "Enable code signing/notarization")in CMakeLists.txt, or - Simplifying the condition to just check
if(PARTIELS_CODESIGN_WINDOWS_KEYFILE)if notarization support is not needed yet.
| if(PARTIELS_NOTARIZE AND PARTIELS_CODESIGN_WINDOWS_KEYFILE) | |
| if(PARTIELS_CODESIGN_WINDOWS_KEYFILE) |
| function(set_target_resources target resource_type source_path) | ||
| # Parse optional DESTINATION argument | ||
| set(options "") | ||
| set(oneValueArgs DESTINATION) | ||
| set(multiValueArgs "") | ||
| cmake_parse_arguments(ARG "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN}) | ||
|
|
||
| # Determine destination directory | ||
| if(ARG_DESTINATION) | ||
| set(dest_dir "${ARG_DESTINATION}") | ||
| else() | ||
| if(resource_type STREQUAL "PLUGIN") | ||
| set(dest_dir "PlugIns") | ||
| else() | ||
| get_filename_component(dest_dir "${source_path}" NAME) | ||
| # Remove trailing slash if present | ||
| string(REGEX REPLACE "/$" "" dest_dir "${dest_dir}") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
The function lacks input validation. Consider adding checks for:
- Valid
resource_typevalues (FILE, DIRECTORY, PLUGIN) - Existence of
source_path(for FILE and DIRECTORY types) - Valid target name (for PLUGIN type)
Example:
if(NOT resource_type MATCHES "^(FILE|DIRECTORY|PLUGIN)$")
message(FATAL_ERROR "Invalid resource_type: ${resource_type}")
endif()
if(resource_type STREQUAL "FILE" AND NOT EXISTS "${source_path}")
message(FATAL_ERROR "File not found: ${source_path}")
endif()| if(NOT resource_type STREQUAL "PLUGIN") | ||
| if(WIN32) | ||
| if(resource_type STREQUAL "FILE") | ||
| get_filename_component(file_name "${source_path}" NAME) |
There was a problem hiding this comment.
The FILE installation on Windows doesn't preserve the directory structure correctly. The code gets file_name from the source path but then installs to ${dest_dir}, meaning a file will be installed as ${dest_dir}/${file_name}. However, the destination should just be the parent directory. Consider:
install(FILES "${source_path}" DESTINATION ${dest_dir} COMPONENT Runtime)This is correct as written (CMake's install(FILES ...) automatically extracts the filename), but the unused file_name variable on line 107 is misleading and should be removed.
| get_filename_component(file_name "${source_path}" NAME) |
| file(APPEND ${PARTIELS_PACKAGE_SCRIPT} "\"${SIGNTOOL_EXE_NAT}\" verify /pa \"${PARTIELS_PACKAGE_OUTEXE_NAT}\"\n") | ||
| endif() | ||
| # Add resource files | ||
| set_target_resources(Partiels FILE "${PARTIELS_BINARYDATA_DIRECTORY}/Resource/FactoryTemplate.ptldoc" DESTINATION Templates) |
There was a problem hiding this comment.
[nitpick] The trailing slash in "${PARTIELS_BINARYDATA_DIRECTORY}/Preset/" is significant for CMake's install(DIRECTORY ...) command - it causes the contents of Preset to be installed to the destination, rather than the Preset directory itself. While this appears intentional, consider being consistent with line 241 (Translations also has trailing slash). For clarity, you might want to add a comment explaining this behavior or remove the trailing slashes and adjust the DESTINATION accordingly.
| set_target_resources(Partiels FILE "${PARTIELS_BINARYDATA_DIRECTORY}/Resource/FactoryTemplate.ptldoc" DESTINATION Templates) | |
| set_target_resources(Partiels FILE "${PARTIELS_BINARYDATA_DIRECTORY}/Resource/FactoryTemplate.ptldoc" DESTINATION Templates) | |
| # Note: The trailing slash in the DIRECTORY argument is significant for CMake's install(DIRECTORY ...) semantics. | |
| # It causes the *contents* of the directory to be installed to the destination, rather than the directory itself. |
| # WiX specific settings | ||
| set(CPACK_WIX_UPGRADE_GUID "2BE88D38-04D3-44AE-B6F6-2D78BD410D58") | ||
| set(CPACK_WIX_PRODUCT_GUID "*") # Generate new GUID for each version | ||
| set(CPACK_WIX_PRODUCT_ICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.png") |
There was a problem hiding this comment.
WiX requires icons to be in .ico format, not .png. The CPACK_WIX_PRODUCT_ICON setting with a PNG file will be ignored or cause errors. You need to either:
- Convert
icon_square.pngtoicon_square.ico, or - Create a separate
.icofile for Windows packaging.
| set(CPACK_WIX_PRODUCT_ICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.png") | |
| set(CPACK_WIX_PRODUCT_ICON "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/icon_square.ico") |
| set(CPACK_WIX_UI_BANNER "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/Ircam-logo-noir-RS.bmp") | ||
| set(CPACK_WIX_UI_DIALOG "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/Ircam-logo-noir-RS.bmp") |
There was a problem hiding this comment.
WiX requires UI banners and dialogs to be in BMP format with specific dimensions (493x58 pixels for banner, 493x312 pixels for dialog). The BMP files should be 8-bit or 24-bit. Using the same file for both may not meet the dimension requirements. Consider verifying that Ircam-logo-noir-RS.bmp has appropriate dimensions or creating separate banner and dialog images.
| set(CPACK_WIX_UI_BANNER "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/Ircam-logo-noir-RS.bmp") | |
| set(CPACK_WIX_UI_DIALOG "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/Ircam-logo-noir-RS.bmp") | |
| set(CPACK_WIX_UI_BANNER "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/banner_493x58.bmp") | |
| set(CPACK_WIX_UI_DIALOG "${CMAKE_CURRENT_SOURCE_DIR}/BinaryData/Resource/dialog_493x312.bmp") |
|
Obsolete |
The CMakeLists.txt had become cluttered with 80+ lines of platform-specific packaging code. Windows used InnoSetup with split CMake/ISS configuration, Linux generated bash scripts entirely in CMake, and adding files required modifications in multiple places.
Changes
CPack Configuration
CPack.cmakewith unified packaging for all platformsResource Management Function
set_target_resources()helper function to simplify resource handlingFILE,DIRECTORY, andPLUGINjuce_add_bundle_resources_directory()install()commands for CPack.catcatalog filesCMakeLists.txt
include(CPack.cmake)CI Workflow
cpack -G TGZreplaces custom bash script executioncpack -C $config -G WIXreplaces InnoSetup, output now.msiinstead of.exeCleanup
BinaryData/Resource/windows-package.issBinaryData/Resource/linux-install.sh(embedded in TGZ packages)Usage
Adding Resources
The new
set_target_resources()function simplifies adding files to packages:This single call handles bundle embedding (macOS), build directory copying (Windows/Linux), and CPack installation rules for all platforms.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.