Move StringTheory to Universal Binary#4
Conversation
Add MultiArch support for universal binary
Fix deprecated/removed methods from Qt5 to Qt6
Move CI builds from Travis/Appveyor to Github actions: - Remove Travis and Appveyor, add Github action file - Switch create-dmg to submodule and update to run on Apple Silicon
cw
left a comment
There was a problem hiding this comment.
Thanks for pushing this modernization forward. I pulled the branch locally and built it end-to-end on macOS; the universal binary claim holds up — Mach-O universal binary with 2 architectures: [x86_64] [arm64], launches cleanly, UI works. CI on your fork is also green (the 5 most recent runs on macos-universal all pass and produce the expected dmg/exe artifacts), which is why no checks are visible on the PR page — Actions isn't yet enabled on the ETCLabs/StringTheory side.
A few notes inline, plus one open discussion question below. Nothing here is a merge blocker — filing as a review comment so you can accept/decline each as you see fit.
Open question: Qt version target (and, relatedly, build system)
Curious what drove Qt 6.9.3 specifically — local dev, most-recently-tested, or arbitrary? Flagging because this PR effectively sets ETCLabs' first Qt 6 target (no other ETCLabs repo is on Qt 6 yet; ETCDmxTool is still on 5.15.2, others still on Travis/AppVeyor + Qt 5.x).
Two reasonable angles on the version itself:
- Qt 6.8 LTS — the conservative pick. Longer documented lifespan, more bug-fix point releases before stream freeze, widely deployed. Useful hedge if ETCLabs plans to port other projects to Qt 6 against the same stream. (Caveat: the extended-LTS support window is commercial-only for open source, so "LTS" here is more about "the more-polished stream" than extra upstream fixes.)
- Latest 6.x — 6.9 is already past its standard 6-month open-source support window as of early 2026. If the goal is "newest," bumping to 6.10 or 6.11 would be more current; I did hit a minor qmake-build compile wart on 6.11 on macOS arm64 that may or may not reproduce on cmake — worth knowing but not a disqualifier.
No strong push either way — whichever you pick probably becomes the de-facto ETCLabs Qt 6 baseline, so worth an intentional decision.
Related open question (definitely out of scope for this PR, just raising for a future chat): Qt 6 has made cmake the primary/recommended build system; qmake is maintained but secondary, and some of the Qt 6.x rough edges land harder on qmake builds than cmake ones. Since we're already rewriting CI and touching .pro files in this PR, it might be a natural moment to ask whether a cmake migration is on the roadmap — not to block this PR, but to avoid doing two "modernize the build" rounds close together. Happy to defer entirely if that's a bigger conversation for somewhere other than a PR thread.
Using HTTPS avoids the need for github SSH keys when cloning.
6.8.x is a QT LTS release.
The `ilammy/msvc-dev-cmd` repository is abandoned. Replace with more currently maintained version. This updates the workflow to load the MSVC environment variables directly via `vcvars64.bat`.
Fix possible case where invalid pingSuccess could be signalled.
|
Thanks for looking at it. On the two broader topics: Qt VersionI had used 6.9.3 for the not very good reason that I've been using it for other projects recently :-). I did experiment with latest Qt but it turns out that some of the CI tools aren't quite there yet, in particular miurahr/aqtinstall#959 is not resolved in the version of aqtinstall used in github actions yet. Switching to CMakeYes, agreed that modern Qt is going more CMake direction and that's certainly what I would do for a new project. However because this project was working with QMake and I didn't want to make two large changes together, I left it as QMake for now. Agree that switching to CMake would be a good thing to do for the future - will log an issue as a reminder for that. |
cw
left a comment
There was a problem hiding this comment.
Re-pulled the branch and walked the post-review commits — all four of my
prior points landed cleanly:
.gitmodules→ HTTPS (7d5546a)ping.cppnon-greedy regex (f88907f)ping.cpphasMatch()guard, with a nice control-flow tidy-up (cc235aa)- Qt 6.8.3 LTS pin (
44b41bb)
Universal binary still builds clean locally, and your fork's CI is still
green across the matrix. Approving — happy for this to land.
Two small follow-ups, neither blocking:
Nit on f150d5d (drop ilammy/msvc-dev-cmd)
The commit message says "load the MSVC environment variables directly
via vcvars64.bat," but the diff actually swaps in
TheMrMilchmann/setup-msvc-dev@v4. Both are reasonable choices — that
action is actively maintained, so the swap is fine — but it looks like
the message is a leftover from an earlier draft. Either rewording the
message or switching to a direct vcvars64.bat call would match
intent. Whichever's easier; not worth holding the PR for.
Tiny cosmetic in cc235aa
In the new control flow, the exit==0-but-parse-failed path now emits
pingFailure(host, tr("Host Unreachable")), which is slightly
misleading — the host did reply, we just couldn't parse the timing.
Something like tr("Could not parse ping response") would be more
accurate. Pure polish; ignore if you'd rather not churn the string
table.
For the cmake follow-up — happy to chase that as its own issue once you
file it; no need to bundle it here.
Thanks again for the modernization work.
Modernize StringTheory project by: