build: list instance_constraints.hpp, unbreaking configure on master - #275
Merged
Conversation
master fails `cmake` configure on every job and platform:
CMake Error at CMakeLists.txt:321 (message):
morph: these public headers belong to no target's FILE_SET HEADERS ...
include/morph/forms/instance_constraints.hpp
Two changes landed that are individually correct and collide: #248 added
include/morph/forms/instance_constraints.hpp, and #265 added a guard requiring
every public header to be in some target's FILE_SET. Neither PR's CI saw the
other, since each was green against a master without the other.
The guard is not wrong here -- it caught exactly what it exists to catch, a
public header that VERIFY_INTERFACE_HEADER_SETS would never have compiled
standalone while continuing to report success. The header simply needs listing,
which is the one-line fix the error message asks for.
Verified: configure succeeds again; the verify target now builds
instance_constraints.hpp.cxx.o among its objects (so the header is genuinely
checked, not merely silencing the guard); and CLIENT_ONLY plus the
QT/OFFLINE_SQLITE/NET combination configure cleanly.
Worth noting for whoever adds the next header: this failure mode is inherent to
a guard that runs at configure time against the merged tree. A PR adding a
header is green until the guard is on master, and the guard is green until the
header is. Nothing detects the pair before they meet, so the first merge after
either lands is where it surfaces.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
master is currently broken — this unbreaks it
cmakeconfigure fails on every job and platform onmaster:Observed on #263, where all 21 jobs fail at the
Configurestep, and reproducedlocally against
origin/master.Cause: two correct changes that collided
include/morph/forms/instance_constraints.hppFILE_SETEach was green. Neither PR's CI saw the other, because each was tested against a
masterthat did not yet contain the other.The guard is not wrong here. It caught precisely what it exists to catch: a
public header that
VERIFY_INTERFACE_HEADER_SETSwould never have compiledstandalone, while continuing to report success. The fix is the one the error
message asks for — list the header.
Verified
master+ this changeinstance_constraints.hpp.cxx.oamong itsobjects, so the header is genuinely checked rather than the guard merely
silenced
MORPH_CLIENT_ONLY=ONandQT + OFFLINE_SQLITE + NETboth configure cleanlyNote for the next header
This failure mode is inherent to a configure-time guard evaluated against the
merged tree: a PR adding a header is green until the guard is on master, and the
guard is green until the header is. Nothing detects the pair before they meet, so
it surfaces on the first merge after either lands.
That's the cost of the guard catching real omissions, and I'd argue it's the
right trade — but it means the first new header after #265 was always going to
hit this once. This is that once.