Run the BVH path from an installed product, and fix what that found - #152
Merged
Conversation
WORKSPACE.md section 5 ships the motion profiles as product data for one stated reason: a converter with no profile available refuses every file it is given, which would make an artifact-only smoke of the BVH path impossible to pass. That has been a requirement without a test since v0.7.0. ost 0.22.3 supplied the missing mechanism ([[workspace.install_data]], data_files: 3) and report 36 section 4 said in as many words that the staging was what had been proven and not the run. This is the run, and it fails. The script packages the aggregate product, verifies it, installs it to a fresh prefix outside this repository, and drives motion_bvh_convert there with no --profile-dir and no USDVRM_MOTION_PROFILE_PATH. The profiles arrive byte-identically at share/usd-vrm-plugins/profiles/motion/ and the converter refuses the capture anyway. Two checks are worth naming. Every shipped profile is compared byte for byte against profiles/motion/, because the failure that shape replaces was a copy that had stopped being the file scripts/check_motion_profiles.py validates. And once the conversion succeeds the installed profile is moved aside and the same command re-run, which must now refuse -- so "the tool found a profile" cannot pass for "the tool found the one this product ships". It runs in release.yml beside the clean-install smoke, against the archive that lane just proved digest-reproducible rather than a fresh package of its own. That places it in the one workflow no pull request event runs, which is a standing caveat of that lane rather than a new one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
motion_bvh_convert derives its profile directory from its own executable path, and its installed-prefix rule was <exe>/../share/usd-vrm-plugins/profiles/motion. That is correct for a `cmake --install` prefix and for a member archive unpacked on its own, and one directory too shallow for the aggregate product: `ost plugin product install` lands a tool member at <prefix>/tools/<member>/bin/ while the product's own data goes to <prefix>/share/. A converter that finds no profile refuses every file it is given, so the whole BVH path was unusable from a release artifact. Two installed layouts put the data in the same place relative to the prefix and the tool at different depths inside it, so one executable-relative rule can only ever serve one of them. The locator now carries both, and the repository rule that already climbed three directories is expressed in terms of the same prefix rather than repeating the arithmetic. None of this moves the destination. It is stated in openstrata.toml, in the root CMakeLists.txt, in WORKSPACE.md section 5 and in ProfileLocator.h, all four agree, and one of them was describing a different prefix -- which is the argument for the smoke rather than for more review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The passing run converts the committed 17-second mocopi export from the prefix alone: 853 frames at 50 Hz, 22 of 27 joints bound. WORKSPACE.md section 5 said "the layout was agreed and only the staging was missing". The agreement was real and with a different layout -- it was measured on a member archive unpacked on its own -- so the paragraph is replaced with what the run found rather than amended. The BVH track's item closes on two fixes in different layers, and the sequence is the point: the packaging ask was real, closing it was not sufficient, and only running the thing distinguished the two. RELEASE_NOTES_TEMPLATE.md still told a reader that no member carries data and to pass --profile a path. That has been stale since ost 0.22.3 and is now wrong: an installed product carries the profiles and the tool finds them with no flag. The member-archive route still needs the flag, and the template now separates the two. The v0.7.0 condition "both paths running from release artifacts alone" moves to in-progress rather than done: the recorded path runs, and the live path still needs the product composed with mocopi_record's own artifact. What is not claimed: this has been measured on a Windows workstation only. The smoke is a release.yml step, so the first Linux and macOS runs happen at a tag, and current.md's before-the-tag list now says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five review findings, and the first is a defect the fix introduced. Both three-parent search rules -- the installed-product one added here and the repository one it was written beside, which had always been unguarded -- were offered from any executable location. From a `cmake --install` prefix, <prefix>/bin/<exe>, they climb two levels above the prefix, which is where a sibling install of this product puts its own share/. Neither CMake rule that places the profiles is unconditional, so a prefix without its own copy is reachable, and the outcome is not a refusal: it is a conversion reading some other prefix's profile. That is the near-miss the no-default-profile rule exists to prevent, arriving through the search path instead of through a flag. Reproduced before fixing: an executable at a/b/prefix/bin/ with a profile only at a/share/... converted 853 frames instead of refusing. Both rules are now offered only when the executable really is in a tools/<member>/bin/, and the same reproduction now refuses and lists one directory. The `cmake --install` layout and the product layout both still resolve. The other four: * the negative check in the smoke -- its headline assertion -- tested only `returncode == 2`, which convert_main.cpp returns for every refusal about the command. A foreign same-id profile that was malformed would have satisfied it and meant the opposite. Measured: such a profile exits 2 with "expected a closing ']'" and no "was found", so the message is what discriminates, and the check now requires it. * the byte-for-byte profile comparison passed having compared zero files if the glob matched nothing. It now fails on the empty list, verified by calling it against an empty authored directory. * release.yml read .data.product.archive without `// empty`, so a missing key would print "null", dirname would give ".", and the smoke would be aimed at the repository root. Same `// empty` plus guard the staging step below already has. * tools/motionBvh/README.md still documented the four-entry search order, and check_docs.py does not reach it. Also corrects WORKSPACE.md's account of the v0.7.0 measurement: it named <root>/share/... as the first directory searched, which was true of a member archive unpacked on its own and read as a general claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes the v0.8.0 carried item "The profiles reach the product; the smoke that would prove it does not exist" — and the smoke found a defect on its first run.
What was open
WORKSPACE.md §5 ships the motion profiles as product data for one stated reason: a converter with no profile available refuses every file it is given, which would make an artifact-only smoke test of the BVH path impossible to pass. That has been a requirement without a test since v0.7.0.
ost0.22.3 supplied the missing mechanism ([[workspace.install_data]],data_files: 3), and report 36 §4 said in as many words that the staging was what had been proven and not the run.The defect
The profiles installed byte-identically to
share/usd-vrm-plugins/profiles/motion/and the converter refused the capture anyway:ost plugin product installlands a tool member at<prefix>/tools/<member>/bin/while the product's own data goes to<prefix>/share/. The locator's installed-prefix rule was<exe>/../share/…— correct for acmake --installprefix and for a member archive unpacked on its own, one directory too shallow for the product. §5's line "the layout was agreed and only the staging was missing" was true of a member archive, which is the layout that measurement was taken on.Two installed layouts put the data in the same place relative to the prefix and the tool at different depths inside it, so one executable-relative rule can only ever serve one of them. The locator now carries both.
The destination never moved. It is stated in
openstrata.toml, the rootCMakeLists.txt, WORKSPACE.md §5 andProfileLocator.h; all four agree, and one of them was describing a different prefix. That is the argument for the smoke rather than for more review.What the smoke does
scripts/artifact_only_bvh_smoke.py:What makes it artifact-only rather than another way of running the tool:
--profile-diris never passed andUSDVRM_MOTION_PROFILE_PATHis removed from the environment;ost envprints plus the activation the product declares in its ownopenstrata.activation.json— which is what the install contract tells a consumer to use.Two checks are worth naming:
profiles/motion/, because the failure that shape replaces was a copy that had stopped being the filescripts/check_motion_profiles.pyvalidates (report 35 §4);Evidence
ctestcheck_docs.py,check_motion_profiles.py,verify_corpus.pygreenWhat is not claimed
Measured on a Windows workstation only. The smoke is a
release.ymlstep, beside the clean-install smoke and against the archive that lane just proved digest-reproducible — so it inherits that lane's standing caveat: no pull request event runs it, and the first Linux and macOS runs happen at a tag.current.md's Before the tag list now carries a line for exactly that.Also in here
mocopi_record's own artifact.RELEASE_NOTES_TEMPLATE.mdsaid "No member carries data" and told a reader to pass--profilea path. Stale sinceost0.22.3 and now wrong for the product; the template separates the product route from the member-archive route, which still needs the flag.Commits
Run the BVH path from an installed product, which refuses every file— the smoke and its lane step, red at this commit by construction.Find the profile a product installs beside the tool that needs it— the locator.Record what the artifact-only run measured, and where it has not run— the documents.🤖 Generated with Claude Code