Move the GitLab Windows jobs to GLR-UV, and build msvc 192 in CI - #129
Conversation
The per-ABI GLR-py310 / GLR-py313 images are gone, replaced by a single
GLR-UV image carrying uv and pre-loaded 3.10 / 3.13 / 3.14 interpreters.
Those images existed for one reason: to make `python` on PATH the matrix
version. The recipe hands CMake `Python3_EXECUTABLE = sys.executable` and
the generated CMakeLists.txt requires `find_package(Python3 ... EXACT
REQUIRED)`, so the interpreter running conan *is* the ABI being built --
which meant the ABI had to be chosen by picking an image and could not be
changed inside the job. That is what PY_TAG and `image: GLR-${PY_TAG}`
were for, and both are now gone: the job establishes the interpreter
itself with `uv venv --python ${PYTHON_TARGET_VERSION}`, activates it,
and installs conan and xmsconan into it with `uv pip install`. GitLab
runs a job's script lines as one shell, so the activation carries.
`uv --version` runs first so a runner without uv fails on a line that
says so rather than four lines later on an install that reads like a
packaging problem. `tags: WinVM` is unchanged -- the tag routes to the
Windows fleet, `image:` only picks the VM template it starts.
Also drops the hard-coded /c/Users/admin from the deploy job's cache
snapshot in favor of ${HOME}. The conan home is `~/.conan2`, so that
literal was an assumption about the old service account, and under any
other one the job shipped an empty artifact and stayed green -- exactly
the kind of assumption a move to a different machine has to stop making.
Docs: USAGE.md §5.6, §9.x, §10.2 and the troubleshooting list, including
two new entries for the failures this shape can produce.
GLR-UV carries VS2019 alongside VS2022, so the matrix that had to be built by hand on a developer workstation can be built on every pipeline instead. Opt-in: it roughly doubles the Windows half of a pipeline, and only the libraries the VS2019-era desktop products consume need msvc 192 binaries at all. The jobs are emitted beside the msvc 194 pair, not instead of it, and reuse its shape -- same runner, same ABI fan-out, same tag-gated export/restore split. Four things differ, and all four are load-bearing: * `build.py --platform windows_vs2019` selects the matrix. The recipe already forks its third-party stack on compiler.version; what was not reachable from build.py is `apply_boost_defaults=False`, which the legacy boost/1.74.0.3 recipe needs because Conan fails a build outright when a profile sets an option no recipe in the graph defines. Both that and the upload destination are derived from --platform rather than exposed as separate flags: a pipeline cannot then set the matrix and the destination differently. * The aquaveo-vs2019 remote is added with `xmsconan_conan_setup --remote-name ... --append`. The function already took both; only the CLI could not reach them. Appended, not inserted first, so it does not become the first stop for every conan install on a shared runner. * Every deploy step carries --package-query compiler.version=192, which xmsconan_conan_deploy also gained. A runner's Conan cache is per machine, not per job, and both `conan cache save` and `conan upload` match by *reference* -- so an unqueried msvc 192 publish would carry a concurrent msvc 194 job's binaries onto the remote whose only purpose is to keep the two apart, and exit 0 having done it. `cache save` cannot take a query directly, so it resolves one through `conan list` into a package list, in a temp file deleted on both paths. * No wheels. A wheel's tags say nothing about which MSVC built it, so an msvc 192 wheel and an msvc 194 wheel are the same devpi filename and publishing both would have them overwrite each other by upload order. The VS2019 wheels stay a deliberate manual step. VS2019_PLATFORM_KEY moves to constants.py, where the generator and the workstation driver both read it, and the generated build.py carries the three msvc 192 literals rendered in rather than imported -- a new name in xmsconan.constants would otherwise make a freshly generated build.py die on an ImportError against an older installed client. Docs: USAGE.md CLI table, [ci] table, build.py flag table, section 10.2, section 16's premise (which said none of this runs in CI), the required CI variables, and three troubleshooting entries.
dd01421 to
a3b1f28
Compare
|
Review of PR #129 (head dd01421, 14 files, +964/-62) — 19 findings after validation Review: PR #129 — Move the GitLab Windows jobs to GLR-UV, and build msvc 192 in CISummaryWhat changed. 14 files, +964/-62, spanning three surfaces at once: the GitLab CI Jinja template ( Notably, the opt-in is not silently forgiving: Validation. Seven specialists ran; three were skipped for having no matching files and two for an absent project marker. The 13 CRITICAL/MAJOR findings they raised went through an independent validation pass, and 11 were dropped. Several drops were not close calls — the finders' supporting evidence was factually wrong. The one CRITICAL (unpaired Verdict: REQUEST CHANGES (C=0 M=2 m=17) Two specialists returned Major itemsM1: ## VS2019 (msvc 192) Packages
GitHub retired the `windows-2019` runner image, so the msvc 192 binaries the Aquaveo desktop products (GMS/SMS/WMS) consume are built **manually, on a developer workstation with Visual Studio 2019 installed**, and published to a separate Conan remote, `aquaveo-vs2019`.
**None of this runs in CI, by design.** CI is unchanged: it still builds gcc 13 / apple-clang 17 / msvc 194 and publishes to the `aquaveo` remote (the `aquaveo-stable` Artifactory repo). There is no Windows-2019 CI job to restore — the runner image is gone.Change: Rewrite Why: M2: {# :718 — "Conan Deploy - Windows" (msvc 194), no query #}
- xmsconan_conan_deploy << library_name >> ${PACKAGE_VERSION} --restore .export/<< library_name >>-windows-py${PYTHON_TARGET_VERSION}-${PACKAGE_VERSION}.tar.gz --upload
{# :774 — "Conan Deploy - Windows VS2019" (msvc 192), queried #}
- xmsconan_conan_deploy << library_name >> ${PACKAGE_VERSION} --restore .export/<< library_name >>-windows-vs2019-py${PYTHON_TARGET_VERSION}-${PACKAGE_VERSION}.tar.gz --upload --remote << vs2019_remote_name >> --package-query compiler.version=<< vs2019_msvc_version >>Change: Add Why: The template states the premise itself at Minor items
|
gagelarsen
changed the title
M2 was the real defect. The msvc 192 publishes were queried; the msvc 194 ones were not, and the hazard is symmetric. A runner's Conan cache is per machine, and `conan cache save <ref>:*` / `conan upload <ref>` match by reference, so once both toolchains build the same reference on the same fleet, the unqueried side ships the other's binaries -- and that direction is the worse one, because it puts msvc 192 binaries on the production `aquaveo` remote at exit 0. Both Windows jobs now carry `--package-query compiler.version=...`, unconditionally rather than only under the opt-in: a guard present in just the configuration that needs it is one refactor from being dropped from the one that does. Both values are read from the packager's `configurations` rather than written into the template. A stale literal would not fail loudly -- a query naming a version the matrix no longer builds matches nothing, and the job goes green having published no binaries at all. M1: this PR made four in-tree passages false and only fixed one of them. README's VS2019 section, `vs2019_build`'s module docstring, the `windows_vs2019` comment in `packager.configurations`, and `VS2019_REMOTE_NAME`'s in constants all still said msvc 192 cannot be built in CI. README also gains the `[ci].windows_vs2019` row. Minor items: * `_package_list` is a context manager. The ownership contract lived in a docstring, and the cleanup was on `CalledProcessError` alone -- so a `conan` missing from PATH leaked the file the docstring promised would never be left behind. Now covered by a parametrized test for both failures. * `--platform` takes `choices` from the packager's matrix, so a typo fails at parse time rather than after the boost-defaults and upload destination have been decided against a value matching nothing. * The msvc 192 preview test reads the `compiler.version` column instead of searching stdout for "192"; the substring form also passed on an empty matrix, which is the failure most worth catching. * The subprocess PYTHONPATH pin derives the repo root from the test file, not from an imported module's `__file__` -- which resolves into site-packages under an installed layout, defeating the pin. * Whole-command assertions in the conan_deploy tests: the `[:3]`/`[-3:]` slices skipped `-c`, the flag that keeps the list a list of *cached* binaries. One shared recorder helper instead of two copies. * A test that the VS2019 save and restore spell the same tarball, the pairing the Linux jobs already have, because a mismatch fails only at tag time. * `"pip install"` rather than `"install"` when filtering steps; the un-flagged `conan_setup` call asserted by shape; the eleven msvc 192 tests grouped under one class; the `\` continuation restored in conan_setup's usage docstring. Investigated and documented rather than changed: conan 2.31 writes `remotes.json` via a `.tmp` plus `os.replace`, so concurrent jobs cannot tear the file, but nothing locks the read-modify-write and a simultaneous add can lose one remote. The failure is loud and the fix -- a per-job CONAN_HOME -- gives up the shared package cache these builds need, so it is noted at both call sites and in the troubleshooting list instead. Declined, with reasons: marking the preview test `integration` (that marker means real conan + gcovr and multi-minute wall time; this runs no conan and takes a second, and the file already has other unmocked subprocess calls), merging the two `if ci_type == "github"` blocks (both predate this PR -- master has the same four), and splitting the generated build.py `__main__` (pre-existing shape this diff adds ~15 lines to; restructuring the script every repo runs belongs in its own change).
gagelarsen
changed the title
|
Thanks for this — M2 was a real defect and I would have shipped it. Addressed in One thing to flag up front: between your review landing and my seeing it, I briefly force-pushed this branch down to just the uv commit while discussing whether the msvc 192 work was worth its complexity. The decision was to keep it, and M2 — mirror leak, fixedYou were right that the mitigation was one-directional, and right that the open direction is the worse one: msvc 192 binaries onto the production Two decisions worth surfacing:
M1 — stale docs, fixedAll four sites: Your correction about Minor itemsApplied: the Two of your minors were sharper than they looked:
|
2 participants
Two related changes to the generated GitLab pipeline, plus a third commit answering review.
1.
a3b1f28— GLR-UV runner and uvThe per-ABI
GLR-py310/GLR-py313images are gone, replaced by a singleGLR-UVimage carrying uv and pre-loaded 3.10 / 3.13 / 3.14 interpreters.Those images existed for one reason: to make
pythonon PATH the matrix version. The recipe hands CMakePython3_EXECUTABLE = sys.executableand the generatedCMakeLists.txtrequiresfind_package(Python3 ... EXACT REQUIRED), so the interpreter running conan is the ABI being built — the ABI had to be chosen by picking an image and could not be changed inside the job. That is whatPY_TAGandimage: GLR-${PY_TAG}were for, and both are gone:The venv is safe for the pybind path: the recipe's
Python3_INCLUDE_DIRhint comes fromsysconfig.get_path('include'), which under thent_venvscheme resolves to the base installation'sInclude(Python.h present), not an empty.venv/Include. Verified against a real venv.Also drops the hard-coded
/c/Users/adminfrom the deploy job's cache snapshot in favor of${HOME}— an assumption about the retired runner's service account that shipped an empty artifact, greenly, under any other one.2.
dd01421— msvc 192 in CI, behind[ci].windows_vs2019GLR-UVcarries VS2019 alongside VS2022, so the matrix that had to be built by hand on a developer workstation can be built on every pipeline instead. Opt-in, defaulting off: it roughly doubles the Windows half of a pipeline, and only the libraries the VS2019-era desktop products consume need msvc 192 binaries.The jobs are emitted beside the msvc 194 pair and reuse its shape. Four things differ, and all four are load-bearing:
build.py --platform windows_vs2019selects the matrix and derivesapply_boost_defaults=False(the legacyboost/1.74.0.3recipe does not declare the 1.86 options, and Conan fails a build when a profile sets an option no recipe defines) and the upload destination — all from one flag, so the matrix and its destination cannot be set differently.xmsconan_conan_setup --remote-name ... --appendaddsaquaveo-vs2019. Appended, so it does not become the first stop for everyconan installon a shared runner.--package-query compiler.version=...on every Windows publish — see the review response below; this started one-sided and is now symmetric.3.
8779d95— review responseThanks — M2 was a real defect and I would have shipped it.
M2 (mirror leak), fixed. The msvc 192 publishes were queried and the msvc 194 ones were not, and the hazard is symmetric — the unqueried direction is the worse one, since it puts msvc 192 binaries on the production
aquaveoremote at exit 0. Both Windows jobs now carry their own query, on both the save and the upload, unconditionally rather than only under the opt-in: a guard present in just the configuration that needs it is one refactor from being dropped from the one that does. Both values are read from the packager'sconfigurationsrather than written into the template — a stale literal fails silently, since a query naming a version the matrix no longer builds matches nothing and the job goes green having published nothing.M1 (stale docs), fixed. All four locations: README's VS2019 section,
vs2019_build's module docstring, thewindows_vs2019comment inpackager.configurations, andVS2019_REMOTE_NAME's inconstants.py. README also gains the[ci].windows_vs2019row. You were right thatconstants.pywas a fourth site — I had updatedMSVC_VS2019_VERSION's comment and missed the one above it. I skipped adding the four new flags to README's Conan Setup / Deploy sections, as you suggested: README defers flag reference todocs/USAGE.md, which has them.Minor items — all applied except three, and the
remotes.jsonquestion answered:_package_listis now acontextmanager. The cleanup was onCalledProcessErroralone, so aconanmissing from PATH leaked the file the docstring promised would never be left behind. Both failure modes are now parametrized tests.--platformtakeschoicesfrom the packager's matrix.compiler.versioncolumn instead of grepping stdout. Worth flagging that the substring form also passed on an empty matrix, which is the failure most worth catching there.__file__. Good catch — that resolved into site-packages and defeated the pin it was written to provide.test_conan_deploy, and one shared recorder helper instead of two copies."pip install"filter; the un-flaggedconan_setupcall asserted by shape; the eleven msvc 192 tests grouped underTestVs2019Ci; the continuation backslash restored inconan_setup's usage docstring.remotes.json(your open question): conan 2.31 writes it asremotes.json.tmpfollowed byos.replace(conan/api/subapi/remotes.py,_save), so the file cannot be torn — a reader sees one complete document or the other. But there is no lock around the read-modify-write, so two jobs adding remotes in the same instant can lose one of the two additions. Left as is, deliberately: the lost update is loud (the job that lost its remote fails resolving dependencies rather than publishing somewhere wrong), and the fix — a per-jobCONAN_HOME— gives up the shared package cache these Windows builds depend on. Noted at both call sites and in the troubleshooting list.Three declined, with reasons:
integration. That marker is defined for "real conan + gcovr … multi-minute wall time"; this test runs no conan and takes about a second, so marking it would exclude it from the default run for no benefit. The premise that it is the suite's only unmockedsubprocess.runis also not right —test_build_file_generator.pyhas others at:1475and:1792, equally unmarked.if ci_type == "github":blocks. Both predate this PR:origin/masterhas the same four blocks at:242,:351,:374,:544. This diff adds a nestedifinside the existing one, not a new block, so merging them is an unrelated refactor.build.py__main__. Pre-existing shape that this diff adds ~15 lines to. Restructuring the script every repo runs has its own blast radius and belongs in its own change.Ops prerequisites
CONAN_LOGIN_USERNAME_AQUAVEO_VS2019/CONAN_PASSWORD_AQUAVEO_VS2019as GitLab CI variables. Branch pipelines pass without them; only the tag-time deploy fails, so the gap would surface at the first release.[ci].windows_vs2019 = truein the repositories that should get msvc 192. Nothing changes until they do.Testing
2351 passed, 4 skipped; flake8 clean.
conan_deployandconan_setupare at 100% line and branch coverage.Four harder pipeline shapes were rendered and checked end to end (split_tests + coverage + wheel_only,
deploy = false,linux = false, xvfb + shards): every job'sstage:is declared instages:, everyneeds:names a job that exists, and the VS2019 deploy job disappears with[ci].deploy = false.