Skip to content

Make sources self-contained without PCH and unity builds - #256

Merged
Eism merged 4 commits into
musescore:mainfrom
luapmartin:luapmartin/self-contained-includes
Sep 2, 2026
Merged

Make sources self-contained without PCH and unity builds#256
Eism merged 4 commits into
musescore:mainfrom
luapmartin:luapmartin/self-contained-includes

Conversation

@luapmartin

@luapmartin luapmartin commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the discussion in #254: makes the framework build with MUSE_COMPILE_USE_PCH=OFF and MUSE_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:

  • Missing includes (<QLocale>, <QPainter>, <QMetaEnum>, <functional>, ...) in sources written against the PCH's implicit baseline. Includes filesystem.h (also fixed in Let the application own the MUSE_COMPILE_USE_PCH option #254 originally; consolidated here).
  • Missing Qt module links: muse_pch links Qt::Core/Gui/Quick PUBLIC, so PCH-enabled modules silently inherited those Qt modules. Modules now declare their real Qt dependencies (Qt::Quick, Qt::Network, ...), and muse_global links Qt::Qml PUBLIC since its public api/iapiengine.h exposes QJSValue.

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.

  • I signed the CLA
  • The title of the PR describes the problem it addresses.
  • Each commit's message describes its purpose and effects, and references the issue it resolves. If changes are extensive, there is a sequence of easily reviewable commits.
  • The code in the PR follows the coding rules.
  • I understand all aspects of the code I'm contributing and I'm able to explain it if requested.
  • The code compiles and runs on my machine, preferably after each commit individually. I have manually tested and verified that my changes fulfil their intended purpose.
  • No prior attempts to resolve this problem exist, or if they do, I listed them in my PR description and described how I avoided repeating past mistakes.
  • There are no unnecessary changes.
  • I created a unit test or vtest to verify the changes I made (if applicable).

Build configuration

audacity: audacity/audacity/master
audacity platforms: linux_x64
musescore: musescore/MuseScore/main
musescore platforms: linux_x64

@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 7d3f5e7f-6977-4090-a9af-5c4cf29b7547

📥 Commits

Reviewing files that changed from the base of the PR and between 8e17462 and 0306587.

📒 Files selected for processing (17)
  • framework/cloud/musescorecom/musescorecomservice.cpp
  • framework/dockwindow/internal/dockseparator.cpp
  • framework/dockwindow/qml/Muse/Dock/docktoolbarview.h
  • framework/dockwindow_v2/qml/Muse/Dock/docktoolbarview.h
  • framework/interactive/internal/iinteractiveprovider.h
  • framework/interactive/internal/interactive.cpp
  • framework/multiwindows/internal/singleprocess/singleprocessprovider.cpp
  • framework/multiwindows/qml/Muse/MultiWindows/multiinstancesdevmodel.cpp
  • framework/multiwindows/qml/Muse/MultiWindows/multiinstancesdevmodel.h
  • framework/ui/api/themeapi.cpp
  • framework/ui/qml/Muse/Ui/initialletternavigation.cpp
  • framework/ui/qml/Muse/Ui/initialletternavigation.h
  • framework/ui/uimodule.h
  • framework/ui/view/qmldataformatter.cpp
  • framework/ui/view/widgetstyle.cpp
  • framework/uicomponents/qml/Muse/UiComponents/polylineplot.cpp
  • framework/update/internal/appupdatescenario.cpp

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 3c238ae4-d5d8-4537-a416-8cbf100e0c2d

📥 Commits

Reviewing files that changed from the base of the PR and between b6fa1e8 and 8e17462.

📒 Files selected for processing (1)
  • framework/update/CMakeLists.txt

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The 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 8e174

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed 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 rea…
Title check ✅ Passed The title clearly and concisely describes the main change: making sources self-contained when PCH and unity builds are disabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

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: #NNNNN" issue reference is not included.

Full details: Docstring Coverage

Explanation

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)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@luapmartin luapmartin self-assigned this Aug 31, 2026
Comment thread framework/uicomponents/qml/Muse/UiComponents/abstracttableviewmodel.cpp Outdated
Comment thread framework/interactive/qml/Muse/Interactive/interactiveprovidermodel.h Outdated
Comment thread framework/global/CMakeLists.txt Outdated
@luapmartin
luapmartin force-pushed the luapmartin/self-contained-includes branch from 4b2354a to b6fa1e8 Compare September 1, 2026 13:27
@luapmartin

Copy link
Copy Markdown
Contributor Author

comments addressed, thanks a lot

@luapmartin
luapmartin force-pushed the luapmartin/self-contained-includes branch from b6fa1e8 to 8e17462 Compare September 2, 2026 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants