Make sources self-contained without PCH and unity builds - #256
Conversation
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (17)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes add missing standard library and Qt includes to framework headers and source files. They also add Qt QML, Quick, and Network link dependencies to relevant CMake targets. No functional logic or public entity declarations changed. Merge Risk: 🔵 Low · up to The PR makes sources self-contained and declares their Qt dependencies for strict non-PCH/non-unity builds. A bounded build-configuration risk remains: the update target must keep its Qt Network linkage unconditional across supported configurations; the change is otherwise mergeable with owner awareness. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly states the problem, scope, motivation, verification results, and build configuration. It completes the checklist except for the applicable unit test or vtest item, which is reasonably left unchecked for include and build configuration changes. The required "Resolves: Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 20 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4b2354a to
b6fa1e8
Compare
|
comments addressed, thanks a lot |
b6fa1e8 to
8e17462
Compare
Follow-up to the discussion in #254: makes the framework build with
MUSE_COMPILE_USE_PCH=OFFandMUSE_COMPILE_USE_UNITY=OFF— every source and header is now self-contained instead of relying on the PCH force-include or a unity batch neighbor.Two classes of fixes:
<QLocale>,<QPainter>,<QMetaEnum>,<functional>, ...) in sources written against the PCH's implicit baseline. Includesfilesystem.h(also fixed in Let the application own the MUSE_COMPILE_USE_PCH option #254 originally; consolidated here).muse_pchlinksQt::Core/Gui/QuickPUBLIC, so PCH-enabled modules silently inherited those Qt modules. Modules now declare their real Qt dependencies (Qt::Quick,Qt::Network, ...), andmuse_globallinksQt::QmlPUBLIC since its publicapi/iapiengine.hexposesQJSValue.Verified by full strict builds (PCH off + unity off) on macOS (Apple clang / libc++) and Linux (gcc-14 / libstdc++, matching CI). Codestyle check passes.
Possible follow-up: a CI job building the strict configuration would keep this from regressing.
Build configuration
audacity: audacity/audacity/master
audacity platforms: linux_x64
musescore: musescore/MuseScore/main
musescore platforms: linux_x64