Skip to content

Refactor: modernize cmake, boost, qt, and others to the latest or near latest#2463

Open
basisunus wants to merge 160 commits into
masterfrom
refactor
Open

Refactor: modernize cmake, boost, qt, and others to the latest or near latest#2463
basisunus wants to merge 160 commits into
masterfrom
refactor

Conversation

@basisunus

Copy link
Copy Markdown
Contributor

Built on Windows 10 with Visual Studio 2022.
Runs on Windows.
Some issues with vector out of bound, possibly initialization issues and qt related.

basisunus added 2 commits May 11, 2026 15:13
# Conflicts:
#	src/Core/Python/PythonInterpreter.cc
@basisunus

Copy link
Copy Markdown
Contributor Author

python runs without crash on mac

@dcwhite

dcwhite commented May 14, 2026

Copy link
Copy Markdown
Member

python runs without crash on mac

Confirmed on my Macbook Air M4

@dcwhite

dcwhite commented Jun 3, 2026

Copy link
Copy Markdown
Member

@jessdtate @basisunus I have another PR that fixes the tests, and then another one to re-enable the CI test harnesses. Once those are in master, we can merge them into here, and actually test this out systematically.

@dcwhite

dcwhite commented Jun 16, 2026

Copy link
Copy Markdown
Member

To fix the build error:

Now I have a complete picture. Here's what's going on:

Yes, libqwt.a is a static library, and yes it's supposed to be that way — the old branches were literally named qt5-static-6.1.5 / qt6-static-6.2.0, making it explicit.

The PR introduced two interconnected problems:

  1. QWT_DIR is no longer exported. The old QwtExternal.cmake set QWT_DIR (the binary dir) and exported it as a cache variable. Superbuild.cmake:291 still passes -DQWT_DIR:PATH=${QWT_DIR} to the inner SCIRun build, and src/CMakeLists.txt:256 uses QWT_DIR to find QwtConfig.cmake. The new file replaced QWT_DIR with QWT_INSTALL_DIR, QWT_LIBRARY, etc. — but Superbuild.cmake was never updated to pass those new variables. So QWT_DIR is now empty/unset.

  2. The new cmake wrapper repo likely builds a dynamic library. The new QwtExternal.cmake hardcodes libqwt.a on Apple/Linux, but the wrapper at https://github.com/CIBC-Internal/Qwt-cmake-wrapper.git tag v0.1.1 may be producing libqwt.dylib instead — which is why CMake can't find libqwt.a as a build target. The old approach controlled this explicitly via the -static- branch name; the new wrapper's build type isn't controlled by any -DBUILD_SHARED_LIBS=OFF flag in CMAKE_ARGS.

The fix would need to either: (a) add -DBUILD_SHARED_LIBS=OFF to the ExternalProject_Add CMAKE_ARGS to force static, and (b) restore exporting QWT_DIR or update Superbuild.cmake:291 to pass the new variables (QWT_LIBRARY, QWT_INCLUDE, etc.) to the inner build.

@dcwhite

dcwhite commented Jun 16, 2026

Copy link
Copy Markdown
Member

There's the answer. The --debug flag in the coverage workflow sets CMAKE_BUILD_TYPE=Debug. Now look at what that does in the new QwtExternal.cmake:

BUILD_COMMAND   ${CMAKE_COMMAND} --build . --config ${_EP_CFG}
INSTALL_COMMAND ${CMAKE_COMMAND} --install . --config ${_EP_CFG}

With _EP_CFG="Debug", the install command becomes cmake --install . --config Debug. On macOS with a single-config generator (Ninja/Makefiles), the Qwt cmake wrapper likely puts the library in a config-specific subdirectory like lib/Debug/libqwt.a — but QwtExternal.cmake hardcodes the path as ${QWT_LIBRARY_DIR}/libqwt.a (no config subdir). So the file is built but installed to a different path than where SCIRun looks for it.

The GUI builds use Release as the build type, and the wrapper apparently does install to lib/libqwt.a flat for Release (or skips the config subdir for Release). The coverage Debug build installs to lib/Debug/libqwt.a or similar, leaving lib/libqwt.a missing.

The fix would be to either:

  • Make the QWT_LIBRARY path config-aware (use a generator expression or $<CONFIG> subdir), or
  • Force single-config flat install in the wrapper with -DCMAKE_INSTALL_PREFIX and no per-config subdir

@basisunus

Copy link
Copy Markdown
Contributor Author

I was trying to replicate it on my mac. I pulled the latest refactor and enabled BUILD_TESTING and ENABLE_CONVERAGE. It builds without the Qwt error. Did I miss anything?

@basisunus

Copy link
Copy Markdown
Contributor Author

Now I see it. Building with Debug now.

@dcwhite

dcwhite commented Jun 17, 2026

Copy link
Copy Markdown
Member

@basisunus Looks like that change broke a bunch of other builds...

This reverts commit a5d5c02.

Revert "improves qwt lib path for multi configs"

This reverts commit 18cd8bb.
@dcwhite

dcwhite commented Jun 18, 2026

Copy link
Copy Markdown
Member

Back to green! Now we need to run all the tests locally, to compare results. I'll set up a shared tracker.

@dcwhite

dcwhite commented Jun 18, 2026

Copy link
Copy Markdown
Member

Sorry for this late-night drollery, but I reread the PR comment and laughed since the title is "Refactor to latest" and the comment says Windows 10 and VS 2022, which are very far from the latest. Next PR: Windows 11 and VS 2026.

@dcwhite dcwhite left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Automated review by Claude (Claude Code), posted via @dcwhite.

Focused pass on build-settings preservation in this refactor. Good news first: no compiler warning flags or compiler/standard flags are removed or changed/wd4251 /wd4275, -Wall -Wunused-parameter -Winfinite-recursion, the C++17 standard settings, and the coverage flags in src/CMakeLists.txt are all untouched. The bulk of the diff is Boost/Python link-target modernization (${SCI_BOOST_LIBRARY} → imported Boost::* targets), not a flags change.

Two minor, non-blocking nits flagged inline below.


# Defensive: prevent accidental linkage to release Python
target_link_options(Core_Python PRIVATE
$<$<CONFIG:Debug>:/NODEFAULTLIB:python313.lib>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Claude review: the Python import-lib version is hardcoded here as python313.lib. When the bundled Python is bumped to 3.14+, this /NODEFAULTLIB:python313.lib will silently stop matching and quietly re-introduce the exact release/debug ABI mixing this block exists to prevent — with no build error to signal it.

Suggest deriving it from the Python version the build already knows, e.g. /NODEFAULTLIB:python${SCI_PYTHON_VERSION_SHORT_WIN32}.lib (or whichever version variable is in scope here), so it tracks the actual Python version automatically.

# if(NOT Qt_PATH OR NOT IS_DIRECTORY "${Qt_PATH}")
#
# if(APPLE)
# set(_qt_default "/Users/basisunus/Qt/6.10.2/macos")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Claude review: this commented-out Qt auto-detection block bakes in developer-specific absolute paths (/Users/basisunus/Qt/6.10.2/macos, plus the specific C:/Qt/6.10.1/... and $ENV{HOME}/Qt/6.11.0/... paths just below). Even commented out, it will mislead other contributors and rot fast as Qt versions move.

Suggest either removing the block entirely or, if the auto-detect convenience is wanted, replacing the hardcoded prefixes with a generic/env-based default before merge.

@dcwhite

dcwhite commented Jun 18, 2026

Copy link
Copy Markdown
Member

🤖 Follow-up from the Claude review (posted via @dcwhite).

I empirically verified the "flags preserved" claim from my earlier review: generated compile_commands.json for both a baseline build and this refactor branch, then diffed the actual per-file compile commands across 1,390 common translation units.

Result: warning flags and compiler/codegen flags are byte-for-byte identical across every file. The only deltas are preprocessor defines, all of which are intentional consequences of the Boost/Qt/Python modernization. Full breakdown in the comment below. 👇

@dcwhite

dcwhite commented Jun 18, 2026

Copy link
Copy Markdown
Member

🤖 Empirical verification of compiler flags / warning settings (Claude review, via @dcwhite)

Method: built compile_commands.json for a baseline tree and this refactor tree with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON, then diffed the actual per-file compile commands. 1,390 common translation units compared.

Warning & compiler flags: identical ✅

Category Result across 1,390 files
-W warning flags Zero differences — no file gains or loses a warning flag
Codegen / language flags (-std=, -O*, -f*, -stdlib=, -arch, -g, …) Zero differences

Boost include treatment: still -isystem

Every Boost include in the refactor build still arrives via -isystem (1,288 files), zero via plain -I — so Boost-header warnings stay suppressed even though the explicit INCLUDE_DIRECTORIES(SYSTEM …) line was removed. The imported find_package(Boost CONFIG) targets mark their includes as system. No regression.

Only preprocessor defines changed — all intentional

Define Direction Reason
BOOST_NO_CXX11_ALLOCATOR, BOOST_PYTHON_STATIC_LIB/MODULE removed old variable-based Boost config; superseded by the new no-lib defines
BOOST_SYSTEM_NO_LIB, BOOST_FILESYSTEM_NO_LIB, BOOST_FILESYSTEM_STATIC_LINK added the new compile definitions in this PR
QWT_DLL (55 files) removed Qwt switched DLL → static
QT_SVG_LIB, QT_PRINTSUPPORT_LIB added now propagated automatically from the Qt::Svg / Qt::PrintSupport imported targets
python3.11python3.13 paths changed refactor bumps Python 3.11 → 3.13 (this is exactly why the hardcoded python313.lib matches today — see inline comment)

Caveat: the baseline tree compared here was a recent non-master feature branch, and the refactor tree includes a later fix-python-test merge. The only 2 files unique to the baseline (OffscreenGLRenderer.cc + its moc_) are a branch-divergence artifact, not a refactor removal.

Net: warning settings and compiler flags are preserved exactly; the refactor changes only which library-selection defines are set, all deliberately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants