Cleanup: Simplify and clean up test123/test123.pro

Open
  • Is something wrong?
#1,729 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
35/100
Issue type
Refactor
Clarity
Mostly clear
Activity status
Active
Tech stack
cpp
Domain
build-system

Research direction

Start with test123/test123.pro; the issue outlines the proposed sources.pri, thirdparty.pri, deploy_macos.pri, and asan.pri split and a four-step implementation plan. First establish a clean-build baseline, then make each planned change in a separate commit, leaving Unix/Linux scoping untouched pending an answer. Done means the cleanup builds successfully and the bundle contents remain equivalent after the QMAKE_BUNDLE_DATA conversion.

Written by the indexing model from the issue text.

Description

Cleanup Ready for final check

🤖 Generated with Claude Code

Summary

Following the Windows-reference removal in #1727, test123/test123.pro was reviewed end to end. It is 830 lines, of which roughly 300 are dead (commented-out blocks, obsolete comments, no-op code) and another 200 are hand-maintained boilerplate that qmake can generate. A cleaned-up equivalent is realistically ~250 lines split across a few .pri files.

What was found

1. The copydata* machinery (biggest maintainability problem)

There are 156 occurrences of copydata in the file. Every deployed file needs four hand-edited lines in three separate places:

copydata11f2.commands = $(COPY_DIR) $$PWD/soundfx/7.short_bell.mp3 $$OUT_PWD/...
first.depends += copydata11f2
export(copydata11f2.commands)
QMAKE_EXTRA_TARGETS += copydata11f2

The names are copydata0a, 0b, 0d, 0e, 1, 1dir, 2, 3, 4s, 5, 10, 11a..11f, 11f2, 11f3, 11g, 11h, 12h, 1p, 2p, 4p, 1sk..4sk, 1tam..5tam, 1vamp..11vamp. The gaps (0c, 2b, 3p, 4-9) are where things were deleted; 11f2/11f3 are where something had to be inserted between 11f and 11g. There is no way to tell what copydata4s does without grepping.

Most of these are plain file copies into the .app bundle, which is what QMAKE_BUNDLE_DATA is for. That would replace ~120 lines with ~10, give correct dependency tracking and incremental copying, and remove every $(MKDIR) / test -d guard and every export().

2. Bugs and no-ops
Line Issue
39-40 set(CMAKE_C_STANDARD 99) / set(CMAKE_CXX_FLAGS ...) are CMake commands. qmake has no set(). Pure no-ops -- the C++ standard they claim to set is not being set.
432, 541 copydata0c.commands is commented out, but export(copydata0c.commands) and first.depends += copydata0c are live, producing an extra-target with an empty recipe.
481 vs 536 copydata10 .. copydata12h are added to first.depends and QMAKE_EXTRA_TARGETS twice.
268 QMAKE_EXTRA_TARGETS = plist uses = not +=. Works only because nothing sets it earlier.
566, 569, 583-585 sleep 2; / sleep 1; / sleep 3; / sleep 5; embedded in copy commands -- race-condition band-aids that add ~16 seconds to every build. The real fix is ordering via .depends.
788 DISTFILES += signSquareDesk.command -- that file does not exist.
795 CONFIG += c++11, while line 40 claims -std=c++17 and Qt 6 requires C++17.
35-41 ARCHDIR is assigned in both arch branches and used only in the message() on line 47.
3. Dead architecture branches

DEFINES += M1MAC=1 is unconditional inside macx{}, and KFR links -lkfr_dsp_neon64, so the x86_64 Mac path is not buildable. Yet the file still carries:

  • the contains(QMAKE_HOST.arch, x86_64) branch (lines 35-41)
  • a ~100-line fully commented-out "NON-M1 MACS ONLY" block (lines 668-770): pocketsphinx, quazip, zlib, BASS
  • soundtouch/source/SoundTouch/cpu_detect_x86.cpp, mmx_optimized.cpp, sse_optimized.cpp in SOURCES -- x86 SIMD files being compiled on arm64
  • commented-out bass_audio.cpp, renderarea.cpp/.h, AppleMusicLibraryXMLReader.cpp inside the SOURCES/HEADERS lists

Git history has all of this; it should be deleted rather than commented.

4. Structure

Five unrelated concerns are interleaved, and three macx{} blocks nest inside another macx{} (lines 316 -> 344, 365). Proposed split:

test123.pro          # ~40 lines: QT, TARGET, CONFIG, includes of the below
  sources.pri        # SOURCES / HEADERS / FORMS / RESOURCES / OBJECTIVE_SOURCES
  thirdparty.pri     # taglib, JUCE, KFR, soundtouch, miniBPM, qpdfjs, frameworks
  deploy_macos.pri   # QMAKE_BUNDLE_DATA, Info.plist, codesign, installer scripts
  asan.pri           # the Issue #1686 block (already well-written, move as-is)

The taglib INCLUDEPATH list is currently duplicated verbatim in the Linux and macOS branches.

5. Hardcoded personal paths

These make the project unbuildable by anyone but the maintainer:

  • $$(HOME)/JUCEProjects/libJUCEstatic/... (three places)
  • codesign --sign 'Apple Development: Michael Pogue (...)'
  • JUCE links Builds/MacOSX/build/Debug -lJUCE_debug even in Release builds
  • QMAKE_MAC_SDK = macosx27.0 pinned in two places

Each should become an overridable variable defaulting to the current value, e.g.:

isEmpty(JUCE_ROOT):   JUCE_ROOT = $$(HOME)/JUCEProjects/libJUCEstatic
isEmpty(CODESIGN_ID): CODESIGN_ID = "Apple Development: ..."
6. Obsolete comments

Lines 380-428 are ~50 lines of troubleshooting notes referencing macOS SDK 10.14/10.15/11.1/11.3, Qt 5.15.2 build directories, and QTBUG-43015. None applies to a Qt 6 / SDK 27 build. The still-useful parts (how to bump QMAKE_MAC_SDK, the stale .qmake.stash gotcha) are worth about 5 lines.

Open question

Is the unix:!macx (Linux) path still real? It is ~25 lines of live config (JUCE static, taglib, kfr, /usr/share/juce). If Linux is dead the way Windows now is, every unix {} block can also collapse -- on macOS unix is always true, so unix {} and macx {} are currently synonyms in this file, which is actively misleading. Step 1 below leaves all Linux/unix scoping untouched pending an answer.

Implementation plan

  1. Delete-only pass -- commented-out blocks, obsolete comments, set() no-ops, copydata0c, the duplicate first.depends, signSquareDesk.command, the x86 branch. Zero behavior change, ~300 lines removed, verifiable with a clean rebuild.
  2. Small correctness fixes -- CONFIG += c++17, QMAKE_EXTRA_TARGETS += for plist, variables for JUCE / codesign / SDK paths.
  3. QMAKE_BUNDLE_DATA conversion -- the real win, but the riskiest; done alone so a before/after diff of bundle contents is meaningful. Dropping the sleeps here recovers the ~16s/build.
  4. .pri split -- mechanical move, last.

Each step is a separate commit so a regression can be bisected to one kind of change.

Dominant language
C++
Stars
14
Forks
4
No merged PRs in 30d

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from mpogue2/SquareDesk

All issues in mpogue2/SquareDesk

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.