ENH: Add migration sentinels for between-tag downstream gating#6665
ENH: Add migration sentinels for between-tag downstream gating#6665hjmjohnson wants to merge 3 commits into
Conversation
938d307 to
26b9351
Compare
Downstream projects build against ITK main between tags, which can be two years apart, and cannot tell whether a specific change is present. Collect one Markdown file per downstream-visible change from CMake/MigrationSentinels into ITK_MIGRATION_SENTINELS and export it through ITKConfig.cmake. Consumers use the list variable rather than a function: CMake's if() cannot invoke a function, and calling one that older ITK does not define is a hard error. An unset variable simply yields false, so an ITK predating this mechanism takes the legacy branch with no guard. The list is exported to CMake only. Embedding it in itkConfigure.h would change every translation unit's preprocessed output on most merges and invalidate ccache fleet-wide; a test asserts it stays out.
Add MigrationSentinels.py with three subcommands: validate, which CI runs so a malformed sentinel cannot merge; expire-itk, which removes every sentinel at a tag because all of them ship in it; and expire-downstream, which reduces a consumer's two-arm condition to its version arm once the tag exists. validate also rejects a misspelled prefix. Such a file is invisible to the ITK_MIGRATION_*.md glob, so the sentinel would silently never be published. expire-downstream pins UTF-8 on the files it rewrites. The locale default is cp1252 on Windows, and the rewrite is in place and not atomic, so an unpinned encoding can silently corrupt non-ASCII content.
Every pull request that added a downstream-visible change to the ITK 6 migration guide gets a sentinel, so downstream projects can gate on any of them before a release contains one. The 6.0 prerelease tags all report ITK_VERSION 6.0.0, so version comparison cannot separate these changes from each other or from current main; the sentinel is the only signal that distinguishes them. Derived by mapping the guide's history to pull requests. Commits that edited the guide without introducing a migration -- a typo fix, a master-to-main prose update, a rewrite of an existing migration's wording, and the commit that created the file -- are excluded.
26b9351 to
91876b4
Compare
|
NOTE: BRAINSia/BRAINSTools@9c86141. Shows how this would be used in downstream projects. |
|
FYI: I think that the migration guide file (which is a single file that often causes merge conflicts) should be an index to the files in commit 91876b4 , and the individual file should contain the report for the migration inducing change. Perhpas add rigid structure to the new topic based files with "Summary", "Expected Migration Actions", "Diagnosis Report" sections. |
|
The solution to this problem is more frequent releases, rather than this unusual solution. Alternatively, perhaps some type of date, or patch number since the prior release could be incorporated is the reported ITK version rather than this. |
|
Thanks @blowekamp — that's a useful pushback I was looking for, and I'd rather get input than decide this myself. Quick notes on the two alternatives you raised: More frequent releases — agreed, and orthogonal. Downstream still builds against Patch count since prior release — tried it, and it fails in exactly the situations that matter. Date in the reported version — this one works, and I think it's a real competing design rather than a variant. CMake's if(ITK_VERSION_FULL VERSION_GREATER_EQUAL 6.0.0.20260719)That drops both release-checklist steps (expiry falls out of version ordering), the two-arm idiom, and nearly all the tooling in this PR. The trade, stated plainly: sentinels answer "is change X present?"; a date answers "is ITK at least as new as the day X landed?" — ordering rather than identity, at day granularity. Two migrations on the same day become indistinguishable. It also requires manually setting the migration day to the day that the PR is merged (or close to it). I don't have a confident view on whether that loss matters in practice, and it's the crux. My original framing came from @thewtex, @dzenanz, @blowekamp — two questions I'd like answered before I build either one:
|
Agreed. Having all migration-related docs in one file is quite useful, even if it is harder to update it.
I like this approach. It is simple. Day granularity is quite enough. And this is indeed a workaround for lack of frequent releases. But I see the use in having it. It is quite useful for downstream projects during pre-release testing, no matter how frequent releases are. |
|
Superseded by #6667, which implements the tweak-date design discussed above. Leaving this open for reference until that lands. |
Downstream projects build against ITK
mainbetween tags, which can be two years apart, and cannot tell whether a specific ITK change is present. This adds migration sentinels: one Markdown file per downstream-visible change, collected intoITK_MIGRATION_SENTINELSand exported throughITKConfig.cmake, so a downstream project can make a fine-grained decision at configure time.The motivating case is
git bisect: a downstream project bisecting ITK to find which change broke it must decide, at each arbitrary commit, which set of baseline images to test against. Sentinels are files in the source tree, so they read correctly in a detachedHEADor a shallow clone — states wheregit describefails outright andgit rev-list --countsilently returns a wrong answer.This is intended only for fine-grained configure-time decisions between tagged releases. It is not a general capability-query API, and it is self-expiring: at each tag the sentinels are deleted and the version arm of the downstream condition takes over.
For maintainers: each release gains two steps (
expire-itk, thenexpire-downstreamon consumers). This PR does not add them to a release checklist — point me at the right file and I will.How a downstream project uses it
Both arms are required. The sentinel arm is true between tags; the version arm is true afterwards, once sentinels have been expired.
Against an ITK that predates this mechanism,
ITK_MIGRATION_SENTINELSis simply unset,IN_LISTyields false, and the legacy branch is taken — no guard, no error. That backward-compatibility path is why this is a list variable rather than a function: CMake'sif()cannot invoke a function, and calling one that older ITK does not define is a hardUnknown CMake commanderror.Why not a version number
Every version-based approach was tried first and fails:
v6.0.0tag, anditkVersion.cmakereports6.0.0atv6.0a01,v6.0b01,v6.0b02and currentmainalike — so version comparison cannot separate any ITK 6 change from any other.--depth 1clone, which CI uses,git describe --tagsfails with "No names found" andgit rev-list --count HEADreturns1— silently wrong, which is worse than an error.VERSION_GREATER_EQUAL 6.0b02is not usable.ccache
The sentinel list must never reach a compiled header.
Modules/Core/Common/src/itkConfigure.h.inis included nearly everywhere; a value that changes on most merges would change every translation unit's preprocessed output and destroy ccache hit rates on every build machine.The list is therefore exported through
ITKConfig.cmakeonly, andMigrationSentinelConfigureHeaderGuardasserts that the generateditkConfigure.hcontains no sentinel — the constraint is enforced by a test, not by a comment.Contents and tooling
CMake/MigrationSentinels/— 33 sentinels, pre-populated from the ITK 6 migration guide's history (46 commits mapped to 37 PRs; 4 excluded as guide maintenance rather than migrations — a typo fix, a master-to-main prose update, a rewrite of an existing migration's wording, and the commit that created the file).CMake/itkMigrationSentinels.cmake— collector; usesCONFIGURE_DEPENDSso adding a sentinel triggers a reconfigure.Utilities/Maintenance/MigrationSentinels.py—validate(CI-enforced),expire-itk,expire-downstream.Documentation/docs/contributing/migration_sentinels.md— contributor guide.Utilities/MigrationSentinelTest/, covering collection, the four-state downstream idiom, the ccache guard, a syntheticfind_package(ITK)consumer, and both the accept and reject paths of the validator.Verification
CONFIGURE_DEPENDSre-triggerITKConfig.cmake; removal reverted itITKConfig.cmakepre-commit run --all-filesAdditive only: no existing behavior changes, and the net diff deletes nothing.