Add Sparkle auto update framework to the Mac build#384
Conversation
…modules, because building them on github is unstable (at least winsparkle). Adding in conan as a package manager now.
… from the github action script. For this, we use find package now instead of hard coding the paths into the third_party subdirectory. Also, bumping version number.
…ckage in the cache?
…the PYTHON_EXECUTABLE?
…ppear multiple times in the DMG created.
…ory, else it doesn't make sense.
…st compiles and runs on my local machine. Now I need to fix the DMG generation, which seems to be broken for a while now.
# Conflicts: # .github/workflows/builds-macos.yml # .github/workflows/builds-windows.yml # .gitmodules # CMakeLists.txt # The-Orm/CMakeLists.txt # third_party/sentry-native
…ly old from Feb 22, and there are changes in there that are no longer desired.
# Conflicts: # CMakeLists.txt # The-Orm/CMakeLists.txt # The-Orm/MainComponent.cpp
# Conflicts: # CMakeLists.txt # The-Orm/CMakeLists.txt
WalkthroughAdds a cross-platform auto-update stack: new C++ AutoUpdaterInterface and macOS Sparkle implementation, per-OS Sparkle integration and packaging changes in The-Orm CMake, top-level CMake/SPARKLE option and FetchContent adjustments, codesign fixes, Makefile configure change, CI steps for Sparkle signing, and a major rewrite of write_appcast.py to generate and upload Sparkle appcasts. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Main as MainComponent
participant Updater as AutoUpdaterInterface
participant Sparkle as Sparkle (SUUpdater) / WinSparkle
App->>Main: initialize()
Main->>Updater: construct (platform-specific)
Note right of Updater #EEF7F6: macOS -> SparkleAutoUpdate (SUUpdater)\nWindows -> win_sparkle_init
alt Startup or user-triggered
App->>Main: checkForUpdatesOnStartup() / "Check for updates"
Main->>Updater: checkForUpdates()
Updater->>Sparkle: request update check
Sparkle-->>Updater: update found / none
Updater-->>Main: notify UI
end
sequenceDiagram
participant CI as Release script
participant Tool as write_appcast.py
participant GH as GitHub API
participant Storage as Release assets / appcast files
CI->>Tool: build_config + add_release(...)
Tool->>Tool: create or ensure local appcast XML
Tool->>GH: get_file_sha (optional)
Tool->>GH: upload_to_github(appcast.xml, release notes html)
GH-->>Storage: updated appcast / notes
Note right of Tool #FFF3E0: Signed DMG handling/signature steps run in CI for macOS tag builds
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
The-Orm/Info.plist.in (1)
6-9: Consider using GitHub Releases for the appcast URL.The
SUFeedURLcurrently points toraw.githubusercontent.com. While this works, consider using GitHub Releases or a CDN for better reliability and security:
- GitHub Releases provide versioned, immutable URLs
- CDNs can offer better availability and caching
- Raw content URLs can be subject to rate limiting
The
SUPublicEDKeyformat appears correct for Sparkle 2.x EdDSA signature verification.The-Orm/MacSparkle.mm (2)
12-12: Consider migrating to the new Sparkle 2.x API.The comment acknowledges this code doesn't follow the new programmatic setup procedure. The current implementation uses deprecated APIs (sharedUpdater, retain/release) which may be removed in future Sparkle versions.
Consider migrating to the modern SPUStandardUpdaterController API documented at https://sparkle-project.org/documentation/programmatic-setup/. This would eliminate the deprecation warnings and future-proof the integration.
23-35: Consider using smart pointers for exception safety.The manual memory management with raw
new/deleteand Objective-Cretain/releasecould leak if an exception is thrown during initialization. While unlikely here, consider wrapping in smart pointers or RAII wrappers for robustness.Apply this pattern for better exception safety:
+#include <memory> + class SparkleAutoUpdate::Impl { public: SUUpdater *updater; + ~Impl() { + if (updater) [updater release]; + } }; SparkleAutoUpdate::SparkleAutoUpdate() { - d = new SparkleAutoUpdate::Impl; + d = new SparkleAutoUpdate::Impl(); d->updater = [[SUUpdater sharedUpdater] retain]; [d->updater setAutomaticallyChecksForUpdates: YES]; [d->updater setUpdateCheckInterval: 3600]; } SparkleAutoUpdate::~SparkleAutoUpdate() { - [d->updater release]; delete d; }The-Orm/MainComponent.cpp (1)
14-14: Consider lazy initialization instead of static construction.The static
sAutoUpdateinstance is constructed beforemain()executes, which can lead to:
- Static initialization order issues if it depends on other globals
- Harder to control initialization timing
- Potential issues if Sparkle needs event loop/application context
Consider lazy initialization:
-static SparkleAutoUpdate sAutoUpdate; +static SparkleAutoUpdate& getAutoUpdater() { + static SparkleAutoUpdate instance; + return instance; +}Then update call sites:
-sAutoUpdate.checkForUpdates(); +getAutoUpdater().checkForUpdates();This ensures initialization happens on first use, after the application is properly set up.
The-Orm/CMakeLists.txt (1)
202-218: Consider using CMake's native framework embedding.The manual framework copying (lines 366-369) and RPATH setup (lines 220-223) could be replaced with CMake's built-in support for embedding frameworks via
MACOSX_BUNDLE_FRAMEWORKSorXCODE_EMBED_FRAMEWORKSproperties (which are commented out on lines 211-213).Consider uncommenting and testing the XCODE properties:
SET_TARGET_PROPERTIES(KnobKraftOrm PROPERTIES MACOSX_BUNDLE_BUNDLE_NAME "KnobKraft Orm ${KnobKraftOrm_VERSION}" MACOSX_BUNDLE_ICON_FILE icon_orm.icns MACOSX_BUNDLE_BUNDLE_VERSION ${KnobKraftOrm_VERSION} MACOSX_BUNDLE_GUI_IDENTIFIER "com.knobkraft.orm" MACOSX_BUNDLE_IDENTIFIER "com.knobkraft.orm" MACOSX_BUNDLE_EXECUTABLE_NAME KnobKraftOrm - #MACOSX_BUNDLE_BUNDLE_VERSION ${KnobKraftOrm_VERSION} MACOSX_BUNDLE_INFO_PLIST ${CMAKE_CURRENT_BINARY_DIR}/Info.plist - #XCODE_EMBED_FRAMEWORKS Sparkle - #XCODE_EMBED_FRAMEWORKS_CODE_SIGN_ON_COPY TRUE - #XCODE_EMBED_FRAMEWORKS_REMOVE_HEADERS_ON_COPY TRUE + XCODE_EMBED_FRAMEWORKS "${SPARKLE_FRAMEWORK_PATH}/Sparkle.framework" + XCODE_EMBED_FRAMEWORKS_CODE_SIGN_ON_COPY TRUE + XCODE_EMBED_FRAMEWORKS_REMOVE_HEADERS_ON_COPY TRUE )This would eliminate the need for manual copying at line 368 and may simplify the RPATH setup.
CMakeLists.txt (1)
149-155: Update Sparkle framework to version 2.8.0.The latest stable Sparkle release is 2.8.0 (released September 16, 2025), but the CMakeLists.txt specifies version 2.6.4. Update the URL to use the current stable version to benefit from recent bug fixes and security improvements:
URL https://github.com/sparkle-project/Sparkle/releases/download/2.8.0/Sparkle-2.8.0.tar.xz
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/builds-windows.yml(1 hunks)CMakeLists.txt(4 hunks)The-Orm/AutoUpdaterInterface.h(1 hunks)The-Orm/CMakeLists.txt(8 hunks)The-Orm/Info.plist.in(1 hunks)The-Orm/MacSparkle.h(1 hunks)The-Orm/MacSparkle.mm(1 hunks)The-Orm/MainComponent.cpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
The-Orm/MacSparkle.h (1)
The-Orm/AutoUpdaterInterface.h (1)
AutoUpdaterInterface(9-14)
🪛 Clang (14.0.6)
The-Orm/AutoUpdaterInterface.h
[error] 9-9: unknown type name 'class'
(clang-diagnostic-error)
[error] 9-9: expected ';' after top level declarator
(clang-diagnostic-error)
The-Orm/MacSparkle.h
[error] 11-11: unknown type name 'class'
(clang-diagnostic-error)
[error] 11-11: expected ';' after top level declarator
(clang-diagnostic-error)
[error] 11-11: unknown type name 'public'
(clang-diagnostic-error)
[error] 11-11: expected ';' after top level declarator
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-ubuntu-24
- GitHub Check: build-ubuntu-22
- GitHub Check: build-windows
- GitHub Check: build-macos
🔇 Additional comments (12)
.github/workflows/builds-windows.yml (1)
75-75: Whitespace formatting only—no functional changes to this workflow.The blank line added after the
sentry-cli upload-difcommand improves readability of the YAML structure. This change is consistent with the PR objective to add Sparkle updates, which primarily affects other parts of the codebase (CMake, C++, and macOS-specific code). The Windows build workflow itself remains functionally unchanged.CMakeLists.txt (2)
7-7: LGTM!The CMake version bump to 3.19 is appropriate for the new features being used (FetchContent improvements and policy CMP0135).
32-34: LGTM!Setting
BUILD_WITH_INSTALL_RPATHto YES is correct for macOS when embedding frameworks like Sparkle. This ensures the framework can be found at runtime via@executable_path/../Frameworks/.The-Orm/AutoUpdaterInterface.h (1)
9-14: LGTM!The abstract interface design is clean and follows best practices with a virtual destructor and pure virtual method. The static analysis errors are false positives from the C parser not recognizing C++ syntax.
The-Orm/MacSparkle.h (1)
11-21: LGTM!The PIMPL pattern is appropriate here to hide Objective-C implementation details from C++ headers. The use of a raw pointer for
dis acceptable since the destructor will handle cleanup (verified in MacSparkle.mm).The-Orm/MacSparkle.mm (1)
28-28: Verify the update check interval aligns with user expectations.The update check interval is set to 3600 seconds (1 hour), which is quite frequent. Consider whether this is the intended frequency or if it should be configurable/longer (e.g., daily checks are more typical).
The-Orm/MainComponent.cpp (3)
6-16: LGTM!The platform-specific Sparkle integration is well-organized with proper preprocessor guards separating Windows and Apple implementations.
433-440: LGTM!The platform-specific update check callbacks are correctly implemented. The lambda capture change to
[](no capture) is appropriate sincesAutoUpdateis a static variable.
629-634: Verify automatic update checks don't interfere with application startup.The Apple path now calls
checkForUpdates()on startup. Ensure this:
- Doesn't block the main thread
- Doesn't show UI during automated startup/testing
- Respects user preferences for automatic checks
Review the Sparkle documentation to confirm that
checkForUpdates()is appropriate for background startup checks vs.checkForUpdatesInBackground()or similar.The-Orm/CMakeLists.txt (3)
51-61: LGTM!The SPARKLE_UPDATES configuration correctly handles both Windows (WinSparkle) and macOS (Sparkle framework) with appropriate platform-specific paths and link flags.
112-116: LGTM!The conditional inclusion of Sparkle sources for macOS is correctly guarded and will only compile the Objective-C++ files when Sparkle updates are enabled.
220-223: LGTM!The post-build RPATH setup using
install_name_toolcorrectly adds@executable_path/../Frameworks/so the application can find the Sparkle framework at runtime.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
The-Orm/CMakeLists.txt (2)
7-7: CMake version requirement increased.The minimum CMake version has been bumped from 3.14 to 3.20. Past reviews noted a mismatch with the root CMakeLists.txt requiring 3.19.
143-143: RPATH configuration conflict persists.This line sets
CMAKE_SKIP_BUILD_RPATH TRUEunconditionally, which conflicts with the root CMakeLists.txt settingBUILD_WITH_INSTALL_RPATH YESfor Apple platforms. This issue was flagged in previous reviews but remains unresolved.
🧹 Nitpick comments (3)
The-Orm/Info.plist.in (1)
6-7: Consider parameterizing the SUFeedURL.The appcast URL is hardcoded to a specific GitHub repository path. If the repository becomes private or the URL structure changes, updates will fail silently for deployed applications.
Consider making this configurable via a CMake variable:
- <string>https://raw.githubusercontent.com/christofmuc/appcasts/master/KnobKraft-Orm/appcast.xml</string> + <string>${SPARKLE_APPCAST_URL}</string>Then set a default in CMakeLists.txt:
set(SPARKLE_APPCAST_URL "https://raw.githubusercontent.com/christofmuc/appcasts/master/KnobKraft-Orm/appcast.xml" CACHE STRING "Sparkle appcast URL")The-Orm/CMakeLists.txt (2)
51-61: Clarify the else branch assumption.The
elsebranch at line 55 assumes non-Windows platforms are macOS with Sparkle framework support. However, the SPARKLE_UPDATES option could theoretically be enabled on Linux, which would cause this code to incorrectly set Sparkle framework variables.Make the platform check explicit:
IF(SPARKLE_UPDATES) if(WIN32) juce_add_binary_data(CodeSigning SOURCES "${CMAKE_CURRENT_LIST_DIR}/../codesigning/dsa_pub.pem") SET(WINSPARKLE_DISTRIBUTION_FILES "${WINSPARKLE_LIBDIR}/WinSparkle.dll") -else() +elseif(APPLE) set(SPARKLE_FRAMEWORK_PATH ${sparkleframework_SOURCE_DIR}) message("Using Sparkle Framework from ${SPARKLE_FRAMEWORK_PATH}") set(SPARKLE_LINK "-F${SPARKLE_FRAMEWORK_PATH}" "-framework Sparkle") +else() + message(FATAL_ERROR "SPARKLE_UPDATES is only supported on Windows and macOS") endif() message("Sparkle and WinSparkle updates are turned on") ENDIF()
366-369: Framework copying may race with build.The
file(COPY ...)command at line 368 copies the Sparkle framework immediately during CMake configuration, not at build time. If the build directory is cleaned or the framework location changes, the copy won't be updated.Use a POST_BUILD command instead to ensure the framework is copied after the target is built:
set(CMAKE_INSTALL_PREFIX ${CMAKE_BINARY_DIR}) IF(SPARKLE_UPDATES) - # Make sure the private frameworks are in out binary directory - file(COPY ${SPARKLE_FRAMEWORK_PATH}/Sparkle.framework DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/KnobKraftOrm.app/Contents/Frameworks) + # Copy Sparkle framework as a post-build step + add_custom_command(TARGET KnobKraftOrm POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_directory + "${SPARKLE_FRAMEWORK_PATH}/Sparkle.framework" + "${CMAKE_CURRENT_BINARY_DIR}/KnobKraftOrm.app/Contents/Frameworks/Sparkle.framework" + COMMENT "Copying Sparkle framework to app bundle" + ) ENDIF()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)The-Orm/CMakeLists.txt(8 hunks)The-Orm/Info.plist.in(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-macos
- GitHub Check: build-ubuntu-24
- GitHub Check: build-windows
- GitHub Check: build-ubuntu-22
🔇 Additional comments (2)
Makefile (1)
38-38: LGTM!The addition of
-DSPARKLE_UPDATES=ONcorrectly enables Sparkle auto-update support in the macOS build configuration.The-Orm/CMakeLists.txt (1)
202-223: Timing concern about Info.plist generation is invalid; no changes needed.The review comment mischaracterizes CMake's execution model. Both
SET_TARGET_PROPERTIES(line 210) andconfigure_file()(line 218) execute during CMake's single configuration phase—not at build time. The file is created before the configuration phase completes, so it will definitely exist when the build system runs. TheMACOSX_BUNDLE_INFO_PLISTproperty simply stores a path reference; it doesn't validate file existence during property assignment. No ordering issue exists here.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
The-Orm/CMakeLists.txt (1)
119-124: Remove duplicate icon configuration.The macOS icon setup is defined twice (lines 119-120 and 123-124) with identical operations. Remove the duplicate block.
Apply this diff:
set(KnobKraftOrm_ICON ${CMAKE_CURRENT_SOURCE_DIR}/resources/icon_orm.icns) set_source_files_properties(${KnobKraftOrm_ICON} PROPERTIES MACOSX_PACKAGE_LOCATION "Resources") - # Mac Icon Magic - set(KnobKraftOrm_ICON ${CMAKE_CURRENT_SOURCE_DIR}/resources/icon_orm.icns) - set_source_files_properties(resources/icon_orm.icns PROPERTIES MACOSX_PACKAGE_LOCATION "Resources")Note: The second block also uses a different path format (
resources/icon_orm.icnsvs${KnobKraftOrm_ICON}), which is inconsistent.
🧹 Nitpick comments (2)
The-Orm/CMakeLists.txt (2)
144-144: Verify conditional SPARKLE_SOURCES handling in add_executable.The
${SPARKLE_SOURCES}variable is used in add_executable, but it's only defined whenSPARKLE_UPDATESis ON and on Apple. When undefined, CMake expands it to an empty string, which is safe. However, explicitly guard this or initialize SPARKLE_SOURCES to an empty list for clarity.Consider adding this before the add_executable call:
if(NOT DEFINED SPARKLE_SOURCES) set(SPARKLE_SOURCES "") endif()Or use conditional target_sources instead:
add_executable(KnobKraftOrm MACOSX_BUNDLE WIN32 ${KnobKraftOrm_ICON} ${SOURCES}) if(SPARKLE_SOURCES) target_sources(KnobKraftOrm PRIVATE ${SPARKLE_SOURCES}) endif()
188-200: Clarify SPARKLE_LINK usage guard for non-Sparkle builds.Line 200 uses
${SPARKLE_LINK}in target_link_libraries, but this variable is only defined whenSPARKLE_UPDATESis ON (line 51-61). On Apple builds without Sparkle enabled, the variable will be undefined. While CMake safely expands undefined variables to empty, add an explicit check for clarity.Wrap the SPARKLE_LINK usage:
ELSEIF(APPLE) if(SPARKLE_UPDATES) target_compile_options(KnobKraftOrm PRIVATE -F${SPARKLE_FRAMEWORK_PATH} -DUSE_SPARKLE) endif() target_link_libraries(KnobKraftOrm PRIVATE ${JUCE_LIBRARIES} ICU::data ICU::uc ${MIDIKRAFT_LIBRARIES} SQLiteCpp sqlite3 gin spdlog::spdlog pybind11::embed $<$<BOOL:${SPARKLE_UPDATES}>:${SPARKLE_LINK}> )This uses CMake's generator expression to conditionally include
SPARKLE_LINKonly when Sparkle is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(4 hunks)The-Orm/CMakeLists.txt(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-ubuntu-24
- GitHub Check: build-macos
- GitHub Check: build-windows
- GitHub Check: build-ubuntu-22
🔇 Additional comments (6)
CMakeLists.txt (1)
81-83: Approve CMake policy and FetchContent setup.The CMake policy CMP0135 (NEW) is required for reproducible builds with FetchContent, and including FetchContent before other configuration is the correct approach.
The-Orm/CMakeLists.txt (5)
7-7: CMake version alignment verified.Version bump to 3.20 now matches the root CMakeLists.txt, resolving the prior version mismatch.
51-61: Sparkle/WinSparkle platform-specific configuration looks correct.The branching properly separates Windows (WinSparkle with DSA keys) from non-Windows (Sparkle framework). However, verify that SPARKLE_LINK is only appended on Apple platforms, as it's defined in the non-Windows else block but later added unconditionally to target_link_libraries.
111-118: CMAKE_SKIP_BUILD_RPATH scoping is now correct.The line is properly placed within the IF(APPLE) block and aligns with the root CMakeLists.txt's
BUILD_WITH_INSTALL_RPATH YESsetting. This resolves the prior conflict concern, and the post-buildinstall_name_toolcommand (lines 220-223) appropriately handles RPATH injection.
366-369: Verify Sparkle framework is available at CMake configure time.Line 368 uses
file(COPY)to embed Sparkle.framework at CMake configuration time. Ensure thatSPARKLE_FRAMEWORK_PATH(set via FetchContent in root CMakeLists.txt) is available and populated by this point in the build process. If FetchContent population is deferred, this copy may fail silently.Consider whether this should be a post-build command instead:
IF(SPARKLE_UPDATES) add_custom_command(TARGET ${PROJECT_NAME} POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_directory "${SPARKLE_FRAMEWORK_PATH}/Sparkle.framework" "${CMAKE_CURRENT_BINARY_DIR}/KnobKraftOrm.app/Contents/Frameworks/Sparkle.framework" COMMENT "Copying Sparkle.framework into app bundle" ) ENDIF()Alternatively, if using configuration-time copy, add a message to confirm the framework exists:
IF(SPARKLE_UPDATES) if(NOT EXISTS "${SPARKLE_FRAMEWORK_PATH}/Sparkle.framework") message(FATAL_ERROR "Sparkle.framework not found at ${SPARKLE_FRAMEWORK_PATH}") endif() file(COPY ${SPARKLE_FRAMEWORK_PATH}/Sparkle.framework DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/KnobKraftOrm.app/Contents/Frameworks) ENDIF()
202-223: Info.plist.in template file exists. Verification confirms that./The-Orm/Info.plist.inexists and theconfigure_file()call on line 218 correctly references it. The macOS bundle configuration is properly set up.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
The-Orm/CMakeLists.txt (1)
121-122: CMAKE_SKIP_BUILD_RPATH conflict remains unresolved.This issue was previously flagged: CMAKE_SKIP_BUILD_RPATH TRUE conflicts with the root CMakeLists.txt BUILD_WITH_INSTALL_RPATH setting and requires a workaround via install_name_tool (lines 220-223). The comment indicates this is intentional for macOS, but the configuration is fragile.
🧹 Nitpick comments (3)
The-Orm/Info.plist.in (2)
10-11: Use CMake variable for CFBundleExecutable.The CFBundleExecutable is hardcoded as "KnobKraftOrm", but CMakeLists.txt line 208 sets MACOSX_BUNDLE_EXECUTABLE_NAME. For consistency and maintainability, use the CMake variable instead.
Apply this diff:
<key>CFBundleExecutable</key> - <string>KnobKraftOrm</string> + <string>${MACOSX_BUNDLE_EXECUTABLE_NAME}</string>
32-32: Remove trailing whitespace.Line 32 contains trailing spaces after
</array>and before</dict>.Apply this diff:
- </array> </dict> + </array> + </dict>The-Orm/CMakeLists.txt (1)
209-213: Remove commented-out code.Lines 209 and 211-213 contain commented-out properties. If these are no longer needed (especially since the Sparkle framework is now copied via post-build command at lines 368-373), remove them to improve maintainability.
Apply this diff:
MACOSX_BUNDLE_IDENTIFIER "com.knobkraft.orm" MACOSX_BUNDLE_EXECUTABLE_NAME KnobKraftOrm - #MACOSX_BUNDLE_BUNDLE_VERSION ${KnobKraftOrm_VERSION} MACOSX_BUNDLE_INFO_PLIST ${CMAKE_CURRENT_BINARY_DIR}/Info.plist - #XCODE_EMBED_FRAMEWORKS Sparkle - #XCODE_EMBED_FRAMEWORKS_CODE_SIGN_ON_COPY TRUE - #XCODE_EMBED_FRAMEWORKS_REMOVE_HEADERS_ON_COPY TRUE )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
The-Orm/CMakeLists.txt(8 hunks)The-Orm/Info.plist.in(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-macos
- GitHub Check: build-ubuntu-22
- GitHub Check: build-windows
- GitHub Check: build-ubuntu-24
🔇 Additional comments (5)
The-Orm/Info.plist.in (1)
6-9: LGTM: Sparkle update configuration is properly set up.The SUFeedURL and SUPublicEDKey are correctly configured for the Sparkle auto-update framework. Using a public EdDSA key in the repository is appropriate since it's used for signature verification.
The-Orm/CMakeLists.txt (4)
51-63: LGTM: Cross-platform Sparkle configuration is well-structured.The conditional logic properly handles Windows (WinSparkle) and macOS (Sparkle framework) with appropriate error handling for unsupported platforms.
188-201: LGTM: Apple Sparkle linking is properly configured.The conditional compilation flags and generator expression for linking are correctly implemented.
215-223: LGTM: Bundle configuration and RPATH setup are correct.The Info.plist template configuration and post-build RPATH addition properly enable the Sparkle framework to be loaded from the app bundle's Frameworks directory.
366-374: LGTM: Sparkle framework bundling is properly implemented.The post-build command correctly copies the Sparkle framework into the app bundle's Frameworks directory when SPARKLE_UPDATES is enabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
The-Orm/CMakeLists.txt (1)
209-213: Remove or clarify the purpose of commented-out XCODE_EMBED_FRAMEWORKS directives.Lines 209–213 contain commented-out XCODE_EMBED_FRAMEWORKS properties. Clarify whether these are placeholders for future work, experimental alternatives, or dead code that should be removed.
If these are not needed, remove them:
SET_TARGET_PROPERTIES(KnobKraftOrm PROPERTIES MACOSX_BUNDLE_BUNDLE_NAME "KnobKraft Orm ${KnobKraftOrm_VERSION}" MACOSX_BUNDLE_ICON_FILE icon_orm.icns MACOSX_BUNDLE_BUNDLE_VERSION ${KnobKraftOrm_VERSION} MACOSX_BUNDLE_GUI_IDENTIFIER "com.knobkraft.orm" MACOSX_BUNDLE_IDENTIFIER "com.knobkraft.orm" MACOSX_BUNDLE_EXECUTABLE_NAME KnobKraftOrm - #MACOSX_BUNDLE_BUNDLE_VERSION ${KnobKraftOrm_VERSION} MACOSX_BUNDLE_INFO_PLIST ${CMAKE_CURRENT_BINARY_DIR}/Info.plist - #XCODE_EMBED_FRAMEWORKS Sparkle - #XCODE_EMBED_FRAMEWORKS_CODE_SIGN_ON_COPY TRUE - #XCODE_EMBED_FRAMEWORKS_REMOVE_HEADERS_ON_COPY TRUE )Alternatively, if they represent intentional fallback strategies or experiments, document their purpose with a clearer comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
The-Orm/CMakeLists.txt(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-ubuntu-22
- GitHub Check: build-windows
- GitHub Check: build-ubuntu-24
- GitHub Check: build-macos
🔇 Additional comments (5)
The-Orm/CMakeLists.txt (5)
113-125: Conditional Sparkle sources and scoped CMAKE_SKIP_BUILD_RPATH are correct.The conditional
SPARKLE_SOURCESsetup (lines 114-120) properly handles macOS Sparkle integration, and the scopedCMAKE_SKIP_BUILD_RPATH(line 122) with its comment aligns with the root CMakeLists.txt'sBUILD_WITH_INSTALL_RPATHsetting for Apple.
144-144: Sparkle sources properly integrated into executable target.Appending
${SPARKLE_SOURCES}to the add_executable call ensures the macOS Sparkle integration files are compiled when enabled.
188-201: Conditional Sparkle compile and link flags are correct.The use of a generator expression at line 200 (
$<$<BOOL:${SPARKLE_UPDATES}>:${SPARKLE_LINK}>) cleanly handles conditional linking. Note that-F${SPARKLE_FRAMEWORK_PATH}appears in both compile options (line 189) and link flags (line 200 via SPARKLE_LINK). While this is defensive and correct, it's mildly redundant—the flag is needed for both compilation (framework header discovery) and linking (framework resolution), so the redundancy is benign.
365-369: Sparkle.framework bundling during installation is correct.The conditional copy of
Sparkle.frameworkinto the app bundle's Frameworks directory ensures the framework is available at runtime. This is the correct pattern for macOS private framework bundling.
51-63: Sparkle integration verified as correct.The
sparkleframework_SOURCE_DIRvariable is properly defined in the root CMakeLists.txt viaFetchContent_DeclareandFetchContent_MakeAvailable(lines 150-154), making it available to The-Orm/CMakeLists.txt at line 56. The platform-specific conditional structure is sound.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/builds-macos.yml(4 hunks)The-Orm/Info.plist.in(1 hunks)write_appcast.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- The-Orm/Info.plist.in
🧰 Additional context used
🪛 Ruff (0.14.2)
write_appcast.py
52-52: Probable use of requests call without timeout
(S113)
72-72: Consider moving this statement to an else block
(TRY300)
83-83: Starting a process with a partial executable path
(S607)
87-87: Consider moving this statement to an else block
(TRY300)
155-155: Probable use of requests call without timeout
(S113)
175-175: Probable use of requests call without timeout
(S113)
253-253: Create your own exception
(TRY002)
253-253: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-ubuntu-24
- GitHub Check: build-macos
- GitHub Check: build-ubuntu-22
- GitHub Check: build-windows
| def get_current_time(): | ||
| now = datetime.now(timezone.utc).astimezone() | ||
| formatted = now.strftime('%Y%m%d %H:%M:%S') | ||
| formatted = now.strftime("%Y%m%d %H:%M:%S") | ||
| tz_offset = int(now.utcoffset().total_seconds() / 3600) | ||
| formatted += '%+d' % tz_offset | ||
| formatted += "%+d" % tz_offset | ||
| return formatted |
There was a problem hiding this comment.
Fix pubDate formatting to Sparkle spec.
get_current_time now emits strings like 20250103 14:08:23-8, but Sparkle parses <pubDate> using the RFC 822 pattern E, dd MMM yyyy HH:mm:ss Z. Without the weekday prefix and zero-padded offset, Sparkle treats the entry as invalid and ignores the release. Please switch back to an RFC 822 formatter, for example:
- formatted = now.strftime("%Y%m%d %H:%M:%S")
- tz_offset = int(now.utcoffset().total_seconds() / 3600)
- formatted += "%+d" % tz_offset
- return formatted
+ return now.strftime("%a, %d %b %Y %H:%M:%S %z")🤖 Prompt for AI Agents
In write_appcast.py around lines 92 to 97, get_current_time currently emits
timestamps like "20250103 14:08:23-8" which do not match Sparkle's RFC‑822
pubDate format; change the function to produce an RFC‑822 style timestamp
(weekday, day month year, time, and a zero‑padded numeric timezone offset) by
using a strftime pattern such as "%a, %d %b %Y %H:%M:%S %z" on a timezone‑aware
datetime so pubDate values look like "Fri, 03 Jan 2025 14:08:23 -0800".
1 participant
Summary by CodeRabbit
New Features
Packaging / Release
Tools