Skip to content

ci: fix nightly quality cross-platform regressions#400

Draft
antoneri wants to merge 2 commits intomasterfrom
codex/fix-nightly-quality-actions
Draft

ci: fix nightly quality cross-platform regressions#400
antoneri wants to merge 2 commits intomasterfrom
codex/fix-nightly-quality-actions

Conversation

@antoneri
Copy link
Copy Markdown
Member

@antoneri antoneri commented Apr 3, 2026

Summary

This PR fixes the three failing job classes in the current Nightly Quality run on master.

It keeps the scope intentionally small:

  • fix macOS C++ correctness configuration on Homebrew/OpenMP runners
  • fix Windows C++ correctness when cmake/ctest resolve to paths containing spaces
  • fix the sanitizer failures that become visible once the build-path issues are out of the way

Root Causes

1. macOS C++ correctness

mk/common.mk called scripts/build_config.py through $(shell ...) and passed CPPFLAGS/CXXFLAGS/LDFLAGS as CLI arguments. On the macOS nightly runner, that quoting path was unstable and repeatedly produced:

build_config.py: error: argument --cppflags: expected one argument

The result was an incomplete CMake configure on macos-15.

2. Windows C++ correctness

mk/test.mk invoked $(CMAKE) and $(CTEST) without quoting. On windows-2025, those variables resolved to paths under C:\Program Files\..., so Git Bash split the command at the space and failed before configure/build.

3. Sanitizers

The Linux sanitizer log showed a leak rooted in generateSubNetwork(Network&), and local sanitizer verification also exposed a second issue in the OpenMP move-commit path:

  • InfoNode::deleteChildren() only deleted active children and ignored collapsedFirstChild / collapsedLastChild
  • tryMoveEachNodeIntoBestModuleInParallel() read m_emptyModules.back() before checking that the vector was non-empty

Changes

  • mk/common.mk
    • stop piping build flags through fragile CLI quoting
    • pass CPPFLAGS / CXXFLAGS / LDFLAGS / MACOSX_DEPLOYMENT_TARGET via environment instead
  • scripts/build_config.py
    • read those values from the environment by default
  • mk/test.mk
    • quote $(CMAKE) and $(CTEST) invocations so paths with spaces remain executable on Windows Git Bash
  • src/core/InfoNode.cpp
    • make deleteChildren() also clean up collapsed child chains
  • src/core/InfomapBase.cpp
    • treat collapsed root children as existing state when reinitializing from a Network
  • src/core/InfomapOptimizer.h
    • guard the OpenMP empty-module check before reading m_emptyModules.back()
  • test/cpp/test_partition.cpp
    • add a regression test that proves deleteChildren() clears collapsed children too

Verification

Local verification run after the final patch set:

  • PATH="/opt/homebrew/bin:$PATH" make doctor MODE=release OPENMP=1 CXX=clang++ CPPFLAGS='-I/opt/homebrew/opt/libomp/include' CXXFLAGS='-I/opt/homebrew/opt/libomp/include' LDFLAGS='-L/opt/homebrew/opt/libomp/lib' MACOSX_DEPLOYMENT_TARGET=15.0
  • PATH="/opt/homebrew/bin:$PATH" make test-native JOBS=1
  • PATH="/opt/homebrew/bin:$PATH" make test-sanitizers JOBS=1
  • PATH="/opt/homebrew/bin:$PATH" make test-native JOBS=1 CMAKE='/tmp/cmake space/cmake' CTEST='/tmp/cmake space/ctest' CTEST_ARGS='-R infomap_cpp_partition_tests'

That last command uses symlinks under a path containing a space to prove the quoted cmake / ctest invocations behave correctly.

Risk

This is still a CI-fix PR, so I’m keeping it draft.

The behavioral changes are intentionally narrow:

  • Make/build-config plumbing only
  • one bounded cleanup fix for collapsed children
  • one bounded OpenMP safety guard in the existing parallel move path

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.

1 participant