Skip to content

Recognize .buildvana/ as an alternative configuration file location - #316

Merged
rdeago merged 8 commits into
Tenacom:mainfrom
rdeago:issue/311-buildvana-directory
Aug 2, 2026
Merged

Recognize .buildvana/ as an alternative configuration file location#316
rdeago merged 8 commits into
Tenacom:mainfrom
rdeago:issue/311-buildvana-directory

Conversation

@rdeago

@rdeago rdeago commented Aug 2, 2026

Copy link
Copy Markdown
Member

Proposed changes

Repos accumulating Buildvana files beyond the configuration file need a way to group them without cluttering the root, following the well-established .github/.config/.claude convention.

The configuration file is now recognized in four candidate locations: buildvana.json[c] in the home directory root (as today) or in a .buildvana subdirectory. More than one existing configuration file is an error naming all offenders — generalizing the existing json/jsonc pairing rule (BVSDK1005 on the SDK side). A configuration file inside .buildvana acts as a home-directory marker for the directory containing .buildvana; a bare .buildvana directory is not a marker. Both discovery implementations (HomeDirectoryDiscovery and the Sdk.props mirror) are updated, and the ComputeVersion cache fingerprint now covers the new locations.

Beyond the issue's scope, by maintainer decision on the PR branch:

  • The .buildvana-home marker file is no longer recognized (breaking change). It predates the configuration file, and a buildvana.json containing {} does the same job while being what one would naturally reach for. Migration: replace .buildvana-home with a buildvana.json containing {}.
  • New Buildvana.Sdk.Tests project: an evaluation harness that copies the real Sdk.props into a stub SDK layout and evaluates it with Microsoft.Build.Evaluation, asserting on $(HomeDirectory) and evaluation errors — the first test coverage for the SDK-side discovery mirror.
  • docs/DirectoryStructure.md synced with the current discovery rules (it still described the Git-only marker set and referenced a diagnostic code and doc file that no longer exist).
  • Suppressed a confirmed ReSharper false positive on CommandParameters.Forwarded (unrelated to these changes; it kept inspectcode from reporting zero).
  • Brought NuGet package dependencies up to date.

Checklist of related issues / discussions

Types of changes

This pull request introduces the following types of changes:

  • Bug fix
  • New feature
  • Test addition / update (no changes to non-test code)
  • Refactor (no changes in public API syntax or semantics)
  • Performance improvement (no changes in public API syntax or semantics)
  • Documentation (docs directory) update
  • Dependency addition / update
  • Changes to the build scripts
  • Changes to CI (workflows, bot / app configurations)
  • Changes to repository files (.gitattributes, .gitignore)
  • Other

Breaking changes

This pull request introduces breaking changes:

  • Yes
  • No

Checklist

  • For all types of changes:
  • For code changes only:
    • The project builds on my machine, via the provided build script, with zero warnings
    • I have added tests that prove my feature works / my fix is effective
    • I have added / modified XML documentation according to changes in code
    • I have checked that all the links I added or modified in XML documentation point to their intended destination
  • For documentation changes (docs directory) only:
    • I have built and tested documentation locally
    • I have checked that all the links I added or modified point to their intended destination

🤖 Generated with Claude Code

rdeago and others added 5 commits August 2, 2026 12:42
Closes Tenacom#311.

The configuration file is now recognized in four candidate locations:
buildvana.json[c] in the home directory root or in a .buildvana
subdirectory. More than one existing configuration file is an error
naming all offenders, generalizing the json/jsonc pairing rule.
A configuration file inside .buildvana marks the directory containing
.buildvana as home; a bare .buildvana directory is not a marker.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New test project Buildvana.Sdk.Tests copies the real Sdk.props into a
stub SDK layout (empty stubs for the build-time generated files and for
ImportConfiguration.props, which would break hermeticity) and evaluates
it with Microsoft.Build.Evaluation via MSBuildLocator. Tests cover home
discovery via every marker, nearest-wins selection, and the BVSDK1005
multiple-configuration-files check.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BREAKING CHANGE: the .buildvana-home marker file is no longer
recognized by home-directory discovery. It predates the configuration
file, and a buildvana.json containing an empty object ({}) does the
same job while being what one would naturally reach for. To migrate,
replace .buildvana-home with a buildvana.json file containing {}.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The section on locating the home directory still described the Git-only
marker set from before configuration files existed, claimed a Git
repository was required, and referenced a diagnostic code and a doc
file that no longer exist. It now describes the nearest-marker
algorithm and the current marker set, and the directory tree shows both
configuration file locations.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NotAccessedPositionalProperty.Global fires on Forwarded although it is
read in the BuildPipeline constructor; confirmed a false positive after
clearing ReSharper caches.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rdeago rdeago added the enhancement [issue/PR] requests / implements new or improved functionality. label Aug 2, 2026
@github-actions github-actions Bot added area:docs [issue/PR] affects documentation (excluding XML documentation that is part of source code). area:code [issue/PR] affects project code (excluding tests). area:deps [issue/PR] adds, updates, or removes dependencies. labels Aug 2, 2026
@rdeago

rdeago commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

@claude please review this PR.

@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Claude finished @rdeago's task in 2m 38s —— View job


Claude finished @rdeago's task

  • Gather context (diff, changed files)
  • Understand request (PR review)
  • Review code changes
  • Post review feedback

Review

Went through the full diff (git diff origin/main...HEAD) — the core logic changes, both test suites, docs, and the changelog. This is a clean, well-tested implementation. No correctness issues found.

Correctness

  • BuildvanaConfigLoader.Load (src/Buildvana.Core.Configuration/BuildvanaConfigLoader.cs:43-60): the four-candidate-array + Array.FindAll generalization of the "both present" check is correct, and the >1 check subsumes the old json/jsonc pairing rule cleanly. Deterministic candidate order (root json → root jsonc → subdir json → subdir jsonc) makes the chosen path and the reported offender list reproducible.
  • HomeDirectoryDiscovery.DirectoryContainsMarker (src/Buildvana.Core.HomeDirectory/HomeDirectoryDiscovery.cs:56-68): correctly treats a bare .buildvana directory as not a marker (only a config file inside it counts) — verified this is exercised by TryDiscover_BareSubdirectory_IsNotMarker.
  • Sdk.props mirror (src/Buildvana.Sdk/Sdk/Sdk.props:38-89): the GetDirectoryNameOfFileAbove calls with a .buildvana/buildvana.json(c) sub-path argument, and the _BV_FoundConfigFile item-count check replacing the old two-condition check, both look right — and unlike most MSBuild logic changes, this one is actually backed by real evaluation tests (Buildvana.Sdk.Tests, using Microsoft.Build.Evaluation against the real Sdk.props copied into a stub SDK layout), not just guessed at. That's a nice addition — this is the first real coverage of the SDK-side discovery mirror, and it closes a class of bugs (MSBuild property/condition typos) that C# tests alone can't catch.
  • ComputeVersion-caching.cs fingerprint (src/Buildvana.Sdk.Tasks/Tasks/ComputeVersion-caching.cs:81-84): correctly extended to include both new candidate paths so the version cache invalidates when a .buildvana/-located config changes.
  • Confirmed no stale references to the removed .buildvana-home marker remain outside the changelog and an explanatory test comment (grepped the whole repo).

Docs

docs/DirectoryStructure.md and docs/SdkDiagnostics.md are accurately synced with the new behavior, including a legitimate bug fix along the way: the "no marker found" error reference changed from the stale BVE1002 (wrong code, wrong doc file) to the correct BVSDK1003 (docs/DirectoryStructure.md:97). Tree diagram ordering (.buildvana\ before artifacts\, buildvana.jsonc before Common.props) is consistent with the existing directories-first-then-files, alphabetical convention.

CHANGELOG entries clearly describe both the new feature and the .buildvana-home breaking change, with migration guidance, matching workflow.md's changelog requirement.

Minor, non-blocking observations

  • src/Buildvana.Tool/CommandLine/CommandParameters.cs:16-22: the #pragma-equivalent // ReSharper disable/restore NotAccessedPositionalProperty.Global wraps the whole positional record, so it also silences the same warning for Options and Positionals, not just Forwarded (the comment only calls out Forwarded as the false positive). Given it's a 3-parameter record where the other two are read elsewhere, the practical risk of this masking a real future warning is low — flagging only for awareness. Fix this →
  • Noticed in passing (unrelated to this PR's diff): docs/DirectoryStructure.md:261 still references BVE1002/ErrorsAndWarnings.md, the same stale pattern this PR just fixed elsewhere in the file — pre-existing, not introduced here, just flagging since it's in a file this PR touches.
  • I couldn't verify whether Microsoft.Build 18.8.2 and Microsoft.Build.Locator 1.11.2 (Directory.Packages.props) are current latest-stable — WebFetch wasn't permitted in this session. Worth a quick manual check against nuget.org per the repo's version-lookup procedure if not already done.

Test coverage

Both new test projects (Buildvana.Core.HomeDirectory.Tests, Buildvana.Sdk.Tests) and the extended BuildvanaConfigLoaderTests cover the new locations, the "not a marker" case, nearest-wins precedence, and the multi-offender error path both in C# and in the real MSBuild evaluation. Solid coverage for a discovery-path change like this.

No blocking issues — this looks ready to merge from a correctness standpoint.

@rdeago

rdeago commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review @claude. I think all your observations have been addressed (the third one by manual check - yes, both added dependencies are at their latest stable version). Please take another look.

@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Claude finished @rdeago's task in 1m 4s —— View job


Claude finished @rdeago's re-review

  • Verify the three prior observations were addressed
  • Re-review full diff for any new issues
  • Post updated review feedback

Re-review

All three observations from the first pass are resolved, and I re-diffed the core logic files (BuildvanaConfigLoader.cs, HomeDirectoryDiscovery.cs, Sdk.props, ComputeVersion-caching.cs) against what I reviewed before — unchanged, so no regressions from the fix-up commits.

  1. CommandParameters.cs suppression scope (commit e4d7f5e) — the // ReSharper disable / restore pair wrapping the whole record was replaced with a single // ReSharper disable once directly above the Forwarded parameter (src/Buildvana.Tool/CommandLine/CommandParameters.cs:20-21). Confirmed Forwarded is indeed read only in BuildPipeline.cs:50, so the comment is now both accurate and minimally scoped — Options/Positionals are no longer incidentally covered. Good fix.
  2. Stale BVE1002/ErrorsAndWarnings.md reference (commit 8d2a338) — docs/DirectoryStructure.md:258 now correctly points to [`BVSDK1002`](SdkDiagnostics.md#buildvana-sdk-core-1000-1049). Verified BVSDK1002 and that anchor both exist in docs/SdkDiagnostics.md:8,36.
  3. NuGet version currency — confirmed manually per your note; I didn't re-verify since you already checked against nuget.org. The unrelated dependency bumps in the same commit (286242f: CliWrap, TUnit, Microsoft.CodeAnalysis.PublicApiAnalyzers, Tools.InnoSetup) are simple version-string changes with nothing to review logically.

No new issues found. This still looks ready to merge from a correctness standpoint.

@rdeago
rdeago merged commit 16d526c into Tenacom:main Aug 2, 2026
6 checks passed
@rdeago
rdeago deleted the issue/311-buildvana-directory branch August 2, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:code [issue/PR] affects project code (excluding tests). area:deps [issue/PR] adds, updates, or removes dependencies. area:docs [issue/PR] affects documentation (excluding XML documentation that is part of source code). enhancement [issue/PR] requests / implements new or improved functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recognize .buildvana/ as an alternative location for Buildvana files

1 participant