feat(package): declarative package management for rbt - #35
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a declarative package management layer for rbt, adding a JSON manifest + lockfile workflow, transitive dependency resolution, multiple fetch sources, and integration into the existing rule-compilation pipeline so package modules participate like local modules.
Changes:
- Add
RBTPackage.json/RBTPackage.lock.jsonmanifest+lock support with a DFS resolver, conflict/cycle detection, and fetchers (git / HTTP archives / path / vcpkg). - Integrate restore into
CppBuildProject.Parse()and synthesize/overlay module rules for binary and upstream-source package shapes. - Add extensive offline test coverage plus new sample projects and updated docs/README.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Sample/PackageConsumer/Source/PackageConsumerTarget.target.cs | New sample target consuming a module that depends on a restored package module. |
| Sample/PackageConsumer/Source/AppModule/Public/AppModule.h | Sample app header. |
| Sample/PackageConsumer/Source/AppModule/Private/AppModule.cpp | Sample app main demonstrating usage of a package-provided header/module. |
| Sample/PackageConsumer/Source/AppModule/AppModule.module.cs | Sample module rule depending on a package module by name. |
| Sample/PackageConsumer/RBTPackage.lock.json | Sample lockfile demonstrating path dependency pinning. |
| Sample/PackageConsumer/RBTPackage.json | Sample manifest declaring a path dependency. |
| Sample/PackageConsumer/global.json | Sample .NET SDK pin for the new sample project. |
| Sample/PackageConsumer/.gitignore | Sample ignore file excluding build outputs and /Packages/. |
| Sample/GeometryPackage/RBTPackage.json | Sample package manifest for a distributable package. |
| Sample/GeometryPackage/Public/GeometryModule.h | Sample package public header with exported API macro usage. |
| Sample/GeometryPackage/Private/GeometryModule.cpp | Sample package implementation. |
| Sample/GeometryPackage/GeometryModule.module.cs | Sample package module rule shipped inside the package. |
| ReBuildTool/ReBuildTool/Program.cs | Adds RunMode.Restore dispatch behavior. |
| ReBuildTool/ReBuildTool.Test/TestPackageVcpkg.cs | Tests vcpkg install-tree → binary-manifest mapping behavior. |
| ReBuildTool/ReBuildTool.Test/TestPackageRestore.cs | End-to-end offline git restore tests using local repos. |
| ReBuildTool/ReBuildTool.Test/TestPackageResolver.cs | Resolver graph-walk tests (transitive, diamond, cycles, conflicts, overrides). |
| ReBuildTool/ReBuildTool.Test/TestPackageManifestEditor.cs | Tests for --PackageAdd/--PackageRemove spec parsing and JSON-preserving edits. |
| ReBuildTool/ReBuildTool.Test/TestPackageManifest.cs | Manifest validation + lockfile roundtrip/write-if-changed tests. |
| ReBuildTool/ReBuildTool.Test/TestPackageBuildIntegration.cs | Integration tests proving restored packages reach rule compilation and toolchain builds. |
| ReBuildTool/ReBuildTool.Test/TestPackageBinaryModule.cs | Tests for synthesized module rules and artifact selection for binary packages + overlays. |
| ReBuildTool/ReBuildTool.Test/TestPackageArchive.cs | Tests archive download/unpack + zip-slip defense + offline re-restore behavior. |
| ReBuildTool/ReBuildTool.Test/TestCppBuild.cs | Adds Sample/PackageConsumer to the sample-build matrix. |
| ReBuildTool/ReBuildTool.Service/PackageService/ProcessRunner.cs | New helper to run external tools and capture stdout/stderr for package operations. |
| ReBuildTool/ReBuildTool.Service/PackageService/PackageRestoreService.cs | Reads manifest, resolves/fetches packages under Packages/, writes lock, updates .gitignore. |
| ReBuildTool/ReBuildTool.Service/PackageService/PackageResolver.cs | DFS dependency resolution, conflict/cycle detection, lock production, overlay resolution. |
| ReBuildTool/ReBuildTool.Service/PackageService/PackageManifestEditor.cs | CLI manifest editing that preserves unknown JSON fields. |
| ReBuildTool/ReBuildTool.Service/PackageService/PackageManifest.cs | Manifest/dep models, pin identity computation, binary-package spec schema, validation. |
| ReBuildTool/ReBuildTool.Service/PackageService/PackageLock.cs | Lockfile schema + read/ignore-future-version + write-if-changed behavior. |
| ReBuildTool/ReBuildTool.Service/PackageService/IPackageService.cs | Package service interface, restore options, restored-package model/result. |
| ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/VcpkgPackageFetcher.cs | vcpkg bridge: bootstrap/install + generate binary-package manifest for installed tree. |
| ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/PathPackageFetcher.cs | Path dependency fetcher (use-in-place, absolute resolved path in resolved). |
| ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/IPackageFetcher.cs | Fetcher interface and fetch request/result models. |
| ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/HttpArchivePackageFetcher.cs | HTTP archive downloader + checksum + atomic unpack + offline behavior. |
| ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/GitPackageFetcher.cs | Git clone/fetch/reset + tag/branch re-resolve (--UpdateLock) + offline behavior. |
| ReBuildTool/ReBuildTool.Service/Context/ServiceContext.Default.cs | Registers IPackageService default implementation in the service context. |
| ReBuildTool/ReBuildTool.Service/CompileService/CppCompile.cs | Marks legacy GitLibraries API obsolete in favor of manifests. |
| ReBuildTool/ReBuildTool.Service/CommandGroup/ICommonCommandGroup.cs | Adds RunMode.Restore CLI mode. |
| ReBuildTool/ReBuildTool.CppCompiler/Project/PackageArgs.cs | Adds restore/edit CLI flags (--Offline, --ForceRestore, --UpdateLock, etc.). |
| ReBuildTool/ReBuildTool.CppCompiler/Project/CppBuildProject.cs | Runs restore before rule glob/compile; globs package rule roots; improves module collision error. |
| ReBuildTool/ReBuildTool.CppCompiler/Package/PackageModuleBinder.cs | Generates .module.cs for binary packages and installs consumer overlays into package roots. |
| ReBuildTool/ReBuildTool.CppCompiler/Package/PackageArtifactSelector.cs | Selects appropriate prebuilt artifacts at Setup() time based on platform/arch/config. |
| ReBuildTool/ReBuildTool.CppCompiler/Common/CppTargetRule.cs | Marks GitLibraries obsolete at target-rule level. |
| ReBuildTool/ReBuildTool.Common/Misc/Hashing.cs | Adds sha256 hashing + tolerant comparison helper. |
| ReBuildTool/ReBuildTool.Common/Misc/Downloader.cs | Adds minimal HTTP download helper with atomic .partial handling. |
| ReBuildTool/ReBuildTool.Common/Misc/ArchiveExtractor.cs | Adds zip/tar/tgz extraction with strip-components and zip-slip prevention. |
| README.md | Mentions package management support at a high level. |
| Doc/HowToUse.zh-CN.md | Documents package management workflow, shapes, lock semantics, and flags; deprecates GitLibraries. |
| Doc/HowToUse.md | English documentation for package management and flags; deprecates GitLibraries. |
| Doc/ARCH.zh-CN.md | Architecture docs updated (package pipeline, rationale, and removed Ini frontend note). |
| Doc/ARCH.md | Architecture docs updated (package pipeline, rationale, and removed Ini frontend note). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void Parse() | ||
| { | ||
| RestorePackages(); | ||
| ParseRules(); | ||
| } |
| var bootstrap = VcpkgRoot.Combine( | ||
| PlatformHelper.IsWindows() ? "bootstrap-vcpkg.bat" : "bootstrap-vcpkg.sh"); | ||
| Log.Info("[package] bootstrapping vcpkg"); | ||
| ProcessRunner.RunOrThrow(bootstrap.ToString(), Array.Empty<string>(), VcpkgRoot, "bootstrapping vcpkg"); | ||
| } |
| process.BeginOutputReadLine(); | ||
| process.BeginErrorReadLine(); | ||
| process.WaitForExit(); | ||
|
|
| return ResolveKind(packageName) switch | ||
| { | ||
| PackageSourceKind.Git => $"git:{Git}@{GitRevision}", | ||
| PackageSourceKind.HttpArchive => $"url:{Url}@{Sha256}", | ||
| PackageSourceKind.Path => $"path:{Path}", | ||
| PackageSourceKind.Vcpkg => $"vcpkg:{Vcpkg}@{Version}:{Triplet}", | ||
| _ => throw new PackageException($"unknown source kind for package \"{packageName}\"") | ||
| }; |
|
Pushed Windows CI failure (3 tests, one root cause). The filter walk climbs from a module directory up until it finds one named This only ever fired on Windows because every other host defaults to the CMake generator, which is why it passed locally and on the Linux/macOS legs. The new
vcpkg triplet in
Local run after the fixes: 100 passed, 2 skipped (Windows-only), 1 failed — Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
ReBuildTool/ReBuildTool.CppCompiler/Project/CppBuildProject.cs:609
- XML doc comment references
RestorePackages, but the method introduced in this PR is namedRestore(). This breaks the cref target (and can fail doc builds when warnings are treated as errors).
/// <summary>Packages materialized by the last <see cref="RestorePackages"/>, in dependency order.</summary>
private List<RestoredPackage> RestoredPackages { get; } = new();
ReBuildTool/ReBuildTool.Service/PackageService/PackageLock.cs:21
- Doc comment says a path dependency's
resolvedvalue is an absolute path, butPathPackageFetcherintentionally returns the declared (portable) path as the resolved string so the lock can be committed and shared. The comment should match the actual semantics.
/// <summary>Commit sha for git, content sha256 for an archive, the absolute path for a path dependency.</summary>
[JsonProperty("resolved")] public string? Resolved { get; set; }
ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/IPackageFetcher.cs:53
FetchedPackage.Resolvedis documented as "an absolute path" for path dependencies, but the fetchers use this string as the lock/identity value andPathPackageFetcherdeliberately returns the declared path (so it stays shareable across machines). Update the comment to avoid misleading API consumers.
/// <summary>Where the package content lives. For a path dependency this is outside Packages/.</summary>
public NPath Root { get; }
/// <summary>What the pin actually resolved to: a commit sha, an archive hash, or an absolute path.</summary>
public string Resolved { get; }
| private readonly HttpListener Listener; | ||
|
|
||
| public LocalServer(NPath file) | ||
| { | ||
| // Port 0 is not available through HttpListener, so probe upward for a free one. | ||
| for (var port = 18800; port < 18900; port++) | ||
| { | ||
| var listener = new HttpListener(); | ||
| listener.Prefixes.Add($"http://127.0.0.1:{port}/"); | ||
| try | ||
| { | ||
| listener.Start(); | ||
| Listener = listener; | ||
| Url = $"http://127.0.0.1:{port}/{file.FileName}"; | ||
| break; | ||
| } | ||
| catch (HttpListenerException) | ||
| { | ||
| listener.Close(); | ||
| } | ||
| } | ||
| if (Listener == null) | ||
| { | ||
| throw new InvalidOperationException("no free loopback port for the test server"); | ||
| } |
|
CI is green on Worth noting: the Linux leg passes Pushed
On Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
ReBuildTool/ReBuildTool.Service/PackageService/ProcessRunner.cs:79
BeginOutputReadLine/BeginErrorReadLineare asynchronous;WaitForExit()can return before the output callbacks have fully drained, which can truncate captured stdout/stderr (especially on failures where you rely onStdErr). Add a second wait (or explicitly wait for stream completion) before returning the captured buffers.
process.BeginOutputReadLine();
process.BeginErrorReadLine();
process.WaitForExit();
ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/HttpArchivePackageFetcher.cs:39
- When
sha256is omitted, the cached-directory fast path always returns success (expected == null), even if the manifest URL changed since the last restore. That can leave a stale package on disk while producing a new lock entry (Origin/Pin updated, content not). The cache hit should be tied to the current lock/pin (or the URL) whensha256is not provided.
{
var current = stamp.ReadAllText().Trim();
if (expected == null || Hashing.Matches(expected, current))
{
return new FetchedPackage(destination, current);
| /// <summary>The matching lock entry, when the project already has one.</summary> | ||
| public LockedPackage? Locked { get; } | ||
|
|
||
| public NPath DefaultDestination => PackagesRoot.Combine(Name); |
| var moduleName = string.IsNullOrWhiteSpace(binary.Module) ? package.Name : binary.Module!; | ||
| var moduleDirectory = packagesRoot.Combine(GeneratedFolderName, moduleName); |
|
Pushed Package name path traversal — real. A dependency keyed Module name injection — real, and the more serious of the two. The name is interpolated into Archive cache with no
Local: 113 passed, 2 skipped (Windows-only), 1 failed — Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
ReBuildTool/ReBuildTool.Service/PackageService/PackageManifest.cs:134
- For vcpkg dependencies, PinKey currently formats as
vcpkg:<port>@<Version>:<triplet>. SinceVersionis typically unset (and isn’t used byVcpkgPackageFetcher), this produces pins likevcpkg:fmt@:x64-windows, which is confusing in lock files and in conflict errors.
return ResolveKind(packageName) switch
{
PackageSourceKind.Git => $"git:{Git}@{GitRevision}",
PackageSourceKind.HttpArchive => $"url:{Url}@{Sha256}",
PackageSourceKind.Path => $"path:{Path}",
PackageSourceKind.Vcpkg => $"vcpkg:{Vcpkg}@{Version}:{EffectiveTriplet}",
_ => throw new PackageException($"unknown source kind for package \"{packageName}\"")
ReBuildTool/ReBuildTool.Service/PackageService/PackageResolver.cs:72
- The returned PackageRestoreResult is not “deepest-first”: packages are added to
Resolvedbefore their dependencies are walked (seeResolved[name] = ...before recursing), soResolved.Valuesenumerates in first-seen order. The current comment is misleading and suggests ordering guarantees that aren’t actually provided.
// Deepest-first: a package is listed after everything it needed, which is the order a
// reader wants and costs nothing to produce here.
return new PackageRestoreResult(Resolved.Values.Select(entry => entry.Package).ToList());
| var moduleName = PackageNames.ValidateModuleName( | ||
| string.IsNullOrWhiteSpace(binary.Module) ? package.Name : binary.Module!, | ||
| package.Name); | ||
| var moduleDirectory = packagesRoot.Combine(GeneratedFolderName, moduleName); | ||
| // The framework unconditionally registers a module's Public/ and Private/ directories; the | ||
| // source globbing warns once per missing path, so create them rather than emit noise. | ||
| moduleDirectory.Combine("Public").EnsureDirectoryExists(); | ||
| moduleDirectory.Combine("Private").EnsureDirectoryExists(); |
|
Pushed Two binary packages claiming one module name — real, and silent, which is the worst part. Both generate to
"Deepest-first" was wrong — correct, a package is recorded before its dependencies are walked, so it precedes them. I checked whether anything actually wanted topological order before changing the comment rather than the code: nothing does, the packages become rule-glob roots and the lock is sorted by name on write. So the comment now states what holds instead of promising a guarantee that was never there. Local: 116 passed, 2 skipped (Windows-only), 1 failed — Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
ReBuildTool/ReBuildTool.Service/PackageService/ProcessRunner.cs:45
ProcessRunner.Run()silently ignores a non-nullworkingDirectorywhen it does not exist, which can cause commands to run in an unexpected directory and make failures very hard to diagnose. If a caller provided a working directory, treat a missing directory as an error.
if (workingDirectory != null && workingDirectory.DirectoryExists())
{
startInfo.WorkingDirectory = workingDirectory.ToString();
}
ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/GitPackageFetcher.cs:130
TryResolve()passes a manifest-controlled revision string togit rev-parsewithout an end-of-options marker. WhileArgumentListprevents shell injection, git still parses leading-as options; adding--avoids option-injection edge cases and makes the call robust.
var result = ProcessRunner.Run(
"git",
new[] { "rev-parse", "--verify", "--quiet", $"{reference}^{{commit}}" },
repository);
| private static string Resolve(NPath packageRoot, string path) | ||
| { | ||
| return System.IO.Path.IsPathRooted(path) ? path : packageRoot.Combine(path).ToString(); | ||
| } |
| ProcessRunner.RunOrThrow( | ||
| "git", | ||
| new[] { "clone", "--recurse-submodules", url, destination.ToString() }, | ||
| null, | ||
| $"cloning package \"{request.Name}\""); |
|
Pushed git option injection via URL and revision — real. Relative artifact paths escaping the package — real. One thing worth flagging on that: I deliberately did not extend this to absolute paths, even though they're equally unconstrained in principle. Absolute entries are exactly what the vcpkg bridge emits — a vcpkg installed tree lives outside
Local: 129 passed, 2 skipped (Windows-only), 1 failed — Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
ReBuildTool/ReBuildTool.Service/PackageService/ProcessRunner.cs:87
ProcessRunner.Runuses asyncBeginOutputReadLine/BeginErrorReadLinebut returns immediately after a singleWaitForExit(). It’s possible for the process to exit before the async readers have drained their final callbacks, which can truncate captured stdout/stderr (and make diagnostics flaky). Add a secondWaitForExit()(or otherwise wait for stream completion) before returning the captured output.
process.BeginOutputReadLine();
process.BeginErrorReadLine();
process.WaitForExit();
ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/VcpkgPackageFetcher.cs:221
DefaultTriplet()currently infers only x86/x64 viaEnvironment.Is64BitOperatingSystem, so on ARM64 hosts it will still return an x64 triplet (e.g.x64-osx). That breaks the intended “default to host triplet” behavior on Apple Silicon / ARM Linux. UseRuntimeInformation.OSArchitecture(or equivalent) to map arm64/arm as well, and consider throwing aPackageExceptionfor unknown architectures so the user is forced to set"triplet"explicitly.
public static string DefaultTriplet()
{
var architecture = Environment.Is64BitOperatingSystem ? "x64" : "x86";
if (PlatformHelper.IsWindows())
{
ReBuildTool/ReBuildTool.Test/TestPackageVcpkg.cs:149
- This test currently asserts the default triplet is only
x86/x64, but the repo claims arm64 host support. OnceDefaultTriplet()is fixed to returnarm64-*on ARM64 machines, this assertion will fail on those hosts. Expand the regex (or make it architecture-agnostic) so the test reflects the intended behavior.
ReBuildTool/ReBuildTool.Service/PackageService/PackageManifestEditor.cs:21 ParseSpec’s docstring and error message describeurl:<href>#<sha256>andvcpkg:<port>#<triplet>as if the#...qualifier is always required, but the implementation allows it to be omitted (URL checksum is optional; vcpkg triplet defaults to host). The message also saysgit:<url>#<tag>even though commit qualifiers are supported. Updating the documentation/error text will prevent confusing UX.
/// Parses the compact spec accepted by <c>--PackageAdd</c>:
/// <c>git:<url>#<tag-or-commit></c>, <c>path:<dir></c>,
/// <c>url:<href>#<sha256></c> or <c>vcpkg:<port>#<triplet></c>.
/// </summary>
ReBuildTool/ReBuildTool.Service/PackageService/PackageManifestEditor.cs:30
- The exception text for malformed specs still implies the
#...qualifiers are mandatory forurl/vcpkg, and that git pins are tag-only. Since the parser acceptsurl:<href>(no sha) andvcpkg:<port>(no triplet), and git accepts tag-or-commit, the error should reflect the actual grammar (e.g.url:<href>[#<sha256>],vcpkg:<port>[#<triplet>],git:<url>#<tag-or-commit>).
throw new PackageException(
$"cannot read package spec \"{spec}\": expected one of " +
$"git:<url>#<tag>, path:<dir>, url:<href>#<sha256>, vcpkg:<port>#<triplet>.");
}
| var request = new FetchRequest( | ||
| name, | ||
| dependency, | ||
| declaringDirectory, | ||
| PackagesRoot, | ||
| Options, | ||
| ExistingLock?.Find(name)); |
|
Pushed A changed pin was silently ignored. The git fetcher reuses the commit the lock recorded rather than asking the remote again — that's what keeps an ordinary build reproducible and offline — but the lock entry was handed over without checking it came from the pin being resolved. So bumping a dependency from
arm64 triplet — real, and I should have caught it myself.
Local: 130 passed, 2 skipped (Windows-only), 1 failed — Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
ReBuildTool/ReBuildTool.Service/PackageService/PackageManifest.cs:140
- For HTTP archive dependencies, the pin key currently ignores the "strip" value. Changing "strip" in RBTPackage.json will not invalidate the lock entry (or conflict detection), even though it changes the extracted tree layout and can break include/source paths.
PackageSourceKind.HttpArchive => $"url:{Url}@{Sha256}",
ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/HttpArchivePackageFetcher.cs:30
- The archive restore fast-path does not consider the manifest's "strip" value. If a user changes stripComponents, restore can incorrectly treat the existing unpacked tree as satisfied and skip re-extraction (since the stamp only keys on hash/URL).
var stamp = request.SidecarFile(StampFileName);
ReBuildTool/ReBuildTool.Service/PackageService/PackageRestoreService.cs:74
- EnsureGitIgnored() claims to be a no-op when the pattern is already present, but it only detects "/Packages/" or "Packages". If a repo already has common variants like "Packages/" or "/Packages", restore will unnecessarily rewrite .gitignore on every first run.
var pattern = $"/{PackagesFolderName}/";
var lines = ignorePath.FileExists()
? ignorePath.ReadAllLines().ToList()
: new List<string>();
if (lines.Any(line => line.Trim() == pattern || line.Trim() == PackagesFolderName))
{
ReBuildTool/ReBuildTool.CppCompiler/Project/CppBuildProject.cs:608
- This comment says RestoredPackages are "in dependency order", but PackageResolver explicitly returns packages in discovery order (parents are recorded before their dependencies). The summary should match the actual ordering to avoid misleading future changes.
/// <summary>Packages materialized by the last <see cref="Restore"/>, in dependency order.</summary>
Projects can now declare external dependencies in an RBTPackage.json next to Source/. rbt resolves them transitively, materializes them under Packages/, and records what each pin actually resolved to in RBTPackage.lock.json. Restore runs from CppBuildProject.Parse(), before ParseRules(). That ordering is forced, not incidental: CompileRules.dll is loaded once with Assembly.LoadFile and cannot be unloaded, so a package's .module.cs has to be on disk before the rule glob runs - there is no second chance to add rules afterwards. The same constraint is why the manifest is JSON rather than C#: resolving transitive dependencies means repeatedly reading the manifest of a package that has not been downloaded yet. Packages contribute modules and extensions, never targets - what gets built stays the consuming project's decision. Their rules are globbed into the same rule assembly as the project's own, so a package module is depended on by name like any local one. Resolution is exact pins only, with no version solver: a git dependency must name a commit, tag or branch, conflicting pins for one package are a hard error pointing at the "overrides" escape hatch, and dependency cycles are reported with the whole chain. The lock stores the commit a tag resolved to, since upstream can move a tag, and is rewritten only when its content changes - rbt's incremental checks are timestamp based. Packages live in <ProjectRoot>/Packages/ rather than under Intermedia/, which Clean() empties and CleanIfNeed() wipes whenever the rbt binaries are newer than the last build; putting them there would re-download every dependency after each rebuild and each rbt update. Sources in this change are git and path. PackageArgs adds --Offline, --ForceRestore and --UpdateLock; it lives in CppCompiler because CmdParser discovers argument groups by scanning loaded assemblies and .NET loads them lazily. RunMode gains Restore. CppTargetRule.GitLibraries, the unread predecessor of all this, is marked [Obsolete]: it hangs off a target rule, which only exists after the rule assembly is compiled, so it never could have worked. A project without an RBTPackage.json is entirely unaffected - no Packages/ directory, no lock file, no .gitignore edit. Tests: manifest validation and lock behaviour, the graph walk (transitive, diamond, cycle, conflict, override), and an end-to-end restore that clones a real git repository created in a temp dir. All offline, so the three CI hosts stay deterministic. Sample/PackageConsumer consumes Sample/GeometryPackage over a path dependency and is compiled and linked for real by the existing sample build suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
Completes the three package shapes and adds the second remote source. HTTP archives (.zip/.tar.gz/.tgz/.tar) become a package source. This needed the three capabilities the repo had nowhere: Downloader, ArchiveExtractor and Hashing in ReBuildTool.Common/Misc. A URL is not self-verifying the way a commit is - the bytes behind it can change without the manifest changing - so a sha256 in the manifest is what makes an archive dependency reproducible, and a mismatch aborts the restore printing both hashes rather than unpacking whatever arrived. Extraction refuses any entry that resolves outside the destination (zip slip), supports strip-components because release tarballs wrap everything in one name-version/ directory, and unpacks through a staging directory so an interrupted run cannot leave a partial tree that a later restore accepts as complete. Prebuilt binary packages ship headers and libraries but no rule, so PackageModuleBinder generates one and PackageArtifactSelector picks the matching artifact by platform/arch/config - an omitted selector matches every value. Selection happens at Setup time rather than when the file is generated: baking the current platform in would change the file's content on every --TargetPlatform switch, and since the rule assembly is rebuilt off rule-file timestamps that would recompile every rule each time. Generated this way the file depends only on the manifest and never churns. Nothing matching is a warning, not a silent empty link. Generating a rule file rather than registering an IModuleInterface straight into ModuleRules keeps everything downstream working unchanged: the ModuleRulePaths lookup in InitAllRule, the SetupInternal lifecycle, the _API macro codegen, all four IDE generators and the HeaderTool plugin. Unmodified upstream source trees ship neither, so the consuming project supplies a rule through the dependency's "overlay". It is copied into the package root, not into a generated directory beside it, because an overlay's relative SourceDirectories/SourceFiles/Exclude* resolve against ModuleDirectory and only mean anything relative to the upstream tree. Tests: archive extraction including zip-slip refusal and strip-components, the download/checksum path served from a loopback HttpListener, artifact selection, and two integration tests that drive a whole project through restore, rule compilation and a real toolchain build - one consuming a binary package, one building upstream sources through an overlay. All offline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
Completes the four package sources and adds command-line manifest editing. The vcpkg fetcher lets vcpkg do the acquiring and building, then translates: it writes an RBTPackage.json describing the installed tree as a prebuilt binary package, so PackageModuleBinder and PackageArtifactSelector take it from there and nothing downstream needs to know vcpkg exists. vcpkg's parallel debug/ prefix is mapped onto rbt's Debug configuration, with an explicit artifact per release configuration - an artifact with no config would also match Debug and both library sets would end up on the link line. A port that built no debug variant has its release libraries serve Debug too, since linking nothing at all is worse. The vcpkg checkout is shared under $RBT_HOME/vcpkg rather than per project: a populated tree is large and slow to rebuild and belongs to no single project. The tool itself is pinned. It clones over https, not the ssh remote the old Actions/Vcpkg helper used - a CI runner or a fresh machine has no ssh key. Triplet comes from the manifest, defaulting to the host's. Restore necessarily runs before any build context exists, so a cross-compiled build has to state its triplet explicitly; the triplet is part of the pin, because two triplets of one port are genuinely different content. --PackageAdd writes a dependency and restores it in one invocation, --PackageRemove drops one. Specs are git:<url>#<tag-or-commit>, path:<dir>, url:<href>#<sha256> and vcpkg:<port>#<triplet>; the qualifier splits off the last '#' because a URL may contain one, and a 40-char hex qualifier is recorded as a commit rather than a tag. Edits go through the raw JSON so fields rbt does not model survive them, and empty fields are not written - a PackageDependency has one field per source kind and all but one are always empty. Tests cover the vcpkg installed-tree mapping against a faked tree (running vcpkg itself needs a large clone, a bootstrap and a network, none of which belongs in a suite that must pass on three CI hosts; the mapping is where the decisions are), and the spec grammar and manifest edits in full. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
Fixes the Windows CI failure and three points from review.
The VS filter walk climbed from a module directory up to the project's Source
folder. Only a module living under Source/ ever reaches that sentinel: a
package's module sits under Packages/, and a package consumed through a path
dependency is not under the project at all - so the walk ran off the top of the
tree and NPath.FileName threw "not valid on a root level directory". It now
stops at the project root and at the filesystem root, and a package outside the
project is grouped under a Modules/ filter by its own directory name rather than
being named relative to the project, which would have produced a "..\..\" filter
Solution Explorer cannot display. This only ever fired on Windows because every
other host defaults to the CMake generator; the new regression test forces VS
generation explicitly, so it now covers all three.
Restore no longer goes through Parse(). Parse() also compiles and loads the rule
assembly and will scaffold a default Target/Module for a project that has none -
neither belongs in "fetch the packages and write the lock, then stop", and the
scaffolding in particular meant --Mode Restore could write source files into a
bare checkout. IProjectInterface gains Restore() and Program.cs dispatches to it
directly.
vcpkg bootstrap runs through cmd.exe /c on Windows. A .bat is not an executable
image, so CreateProcess cannot launch it and ProcessRunner does not use
ShellExecute - as written it would have failed on every Windows host.
An omitted vcpkg triplet now resolves to the host's before pins are compared.
Previously { "vcpkg": "fmt" } and { "vcpkg": "fmt", "triplet": "<host>" } produced
different pin keys and would be reported as conflicting on the very machine where
they are identical, and the lock recorded a pin that did not say which triplet
was built.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
Three comments no longer matched the code they describe: - CppBuildProject's RestoredPackages cref still pointed at RestorePackages, renamed to Restore() in the previous commit, so the cref no longer resolved. - PackageLock.Resolved and FetchedPackage.Resolved both said a path dependency records an absolute path. It records the path exactly as declared - deliberately, since the lock is committed and an absolute path would be meaningless in any other checkout. Both now say what the field actually holds for each source. LocalServer in the archive tests probed for a free port by assigning the readonly field inside the loop and then null-checking a non-nullable field. It compiled (definite assignment is not enforced for class fields) but read as though it might not have. The probe now uses a local and the fields are assigned once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
… URL Three findings from review, two of them about trusting a manifest too far. A package fetched from a remote declares its own transitive dependencies, so the names arriving from it are exactly as trustworthy as the package is - and rbt turns them into filesystem paths and into generated C# it then compiles and runs. Package names are now validated before they are combined into a path. The check sits in the resolver, so it covers every name in the walk rather than only the ones in the project's own manifest, and is re-applied in FetchRequest for any future caller building a request directly. A dependency keyed "../outside" would previously have placed the package next to Packages/ instead of inside it; the sidecar file the archive fetcher writes had the same hole and now goes through the same containment check. Binary-package module names are validated as plain C# identifiers. The name is interpolated into "public class <name>" in a generated rule that becomes part of CompileRules.dll, so a name carrying punctuation could have closed the declaration and appended arbitrary code to an assembly rbt executes during the build. Rejected rather than escaped: a package still has no business naming a class outside its own. The archive fetcher's cache hit is keyed on the origin URL when the manifest gives no sha256. Previously any stamp satisfied that case, so editing a dependency's url left the previous archive unpacked on disk while the lock recorded the new origin - stale content under a fresh-looking pin. The stamp now carries the URL alongside the hash; a stamp from an older rbt simply misses once and re-downloads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
…pkg version Two binary packages could both name their module the same thing. Both generate to Packages/.generated/<module>/<module>.module.cs - one file - so the second write won and the build quietly depended on which package was processed last. The binder now tracks the names it has generated and reports both culprits. The same clash between two source packages was already caught in ParseRules, where they are two distinct files claiming one name; this path had to catch its own. PackageDependency.Version is removed. Nothing ever read it: which version of a vcpkg port you get is decided by the vcpkg checkout, which the fetcher pins to a fixed tag, and real per-port pinning would need vcpkg manifest mode and a versioning baseline that this bridge does not set up. Keeping the field would have left a knob that silently does nothing, and it also made every vcpkg pin read "vcpkg:fmt@:x64-windows" in lock files and conflict messages. Pins are now "vcpkg:<port>:<triplet>". The restore result's ordering comment claimed deepest-first. It is not: a package is recorded before its own dependencies are walked, so it precedes them. Nothing downstream depends on the order - the packages become rule-glob roots and the lock is sorted by name on write - so the comment now says what actually holds rather than promising a guarantee that was never provided. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
Four more from review, all variants of letting a manifest reach further than it should. A git URL or revision out of a manifest becomes an argv element. ArgumentList already rules out shell injection, but git itself still reads a leading '-' as an option, so a value like "--upload-pack=..." would have turned into a git flag. Both are now validated where the rest of the manifest is validated, once, rather than at each site that shells out. Rejecting the shape is portable; --end-of-options would have added a git 2.24 floor for the same effect. git clone also gets an explicit "--" so it does not depend on that check staying in place. A binary package's relative include and library paths could climb out of the package with "../..", putting arbitrary directories of the consuming machine on the include or library search path. Relative entries must now resolve inside the package. Absolute entries stay allowed on purpose - that is what the vcpkg bridge emits, since a vcpkg installed tree lives outside Packages/ by design. ProcessRunner silently ignored a working directory that did not exist, which meant the tool ran in rbt's own working directory instead: a git command against the wrong repository, failing in a way that gives no hint why. A directory the caller named now has to be there. ReBuildTool.Service gains InternalsVisibleTo for the test project, matching what ReBuildTool.CppCompiler already does, so the runner can be covered directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
…plet A changed pin was silently ignored. The git fetcher reuses the commit the lock recorded rather than asking the remote again - that is what keeps an ordinary build reproducible and offline - but the lock entry was handed over without checking it came from the pin being resolved. Bumping a dependency's tag from v1.0 to v2.0 therefore resolved to the commit v1.0 pointed at, which still exists in the clone, and the build stayed on the old version while the manifest said otherwise. The resolver now only passes an entry whose pin matches. DefaultTriplet inferred the architecture from Is64BitOperatingSystem, which only separates 32- from 64-bit, so every arm64 host was handed an x64 triplet - and rbt targets Apple Silicon and arm64 Linux (Vendor/ninja ships a linux-aarch64 binary and CI runs a macOS arm64 leg). It now reads RuntimeInformation .OSArchitecture and throws for an architecture it has no mapping for, rather than installing binaries for the wrong machine and failing somewhere far from the cause. Its test asserted the old x86/x64-only behaviour and now checks the triplet against the actual host. --PackageAdd's grammar text claimed the '#' qualifier is required everywhere and that git pins are tag-only. Only git needs one, having no default revision to fall back on; a url without a sha256 is simply unverified and a vcpkg port without a triplet uses the host's. Docstring and error message now say so. ProcessRunner's WaitForExit gets a comment recording why the parameterless overload is the correct one - it is the version that waits for the redirected streams to drain, which is what keeps captured output complete. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nEaEVhDjn2dQC19BE9iaM
89f82c0 to
b66c3f8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
ReBuildTool/ReBuildTool.Common/Misc/ArchiveExtractor.cs:127
ResolveEntryPathblocks..segments and also checksIsChildOf, but on Windows an entry name containing:(e.g.foo.txt:evil) can target an NTFS alternate data stream. That allows archives to smuggle content in a way that is not visible in Explorer and may bypass expectations about extracted file names. Consider rejecting any segment containing:on Windows (or universally) before combining into the destination path.
if (segments.Any(segment => segment == ".."))
{
throw new IOException(
$"refusing to extract \"{entryName}\": the archive entry escapes the destination directory.");
}
ReBuildTool/ReBuildTool.IDE/VisualStudio/VCProject.Filter.cs:152
- Visual Studio filter generation currently adds all files under
moduleDirectory. With git-sourced packages, the module directory is typically the clone root andGitPackageFetcherkeeps the.gitdirectory; that would pull the entire git object database into the generated project/filters, making the.vcxproj/filters huge and slow (and potentially overwhelming VS). It looks worth excluding.git(and the.gitfile case used by submodules/worktrees) from this enumeration.
moduleDirectory.Files(true).ToList().ForEach(file =>
{
GetOrAddFilter(file.Parent, moduleDirectory).Files.Add(file.RelativeTo(outputFolder));
});
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
ReBuildTool/ReBuildTool.Service/PackageService/ProcessRunner.cs:90
- ProcessRunner uses async OutputDataReceived/ErrorDataReceived handlers but disposes the Process immediately after a single WaitForExit(). WaitForExit() only guarantees the process exited; it can still return before the async handlers have drained, which can truncate the tail of stdout/stderr.
// The parameterless WaitForExit is the overload that also waits for the redirected streams
// to reach EOF and the async handlers above to drain - that is precisely what separates it
// from WaitForExit(int), which returns as soon as the process is gone and can leave the
// last lines uncollected. Do not "optimise" this into the timeout overload.
process.WaitForExit();
ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/HttpArchivePackageFetcher.cs:99
- The stamp file only records (hash, url). If extraction behavior (e.g. strip components) changes, there’s no way to detect it later. Persist strip in the stamp so the cache hit is only taken when both content identity and extraction options match.
stamp.WriteAllText($"{actual}{Environment.NewLine}{url}{Environment.NewLine}");
ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/HttpArchivePackageFetcher.cs:114
- ReadStamp assumes exactly two lines. Once the stamp includes strip (and any future metadata), this should parse the extra fields with a backward-compatible default so older stamps still work.
private static (string Hash, string Url) ReadStamp(NPath stamp)
{
var lines = stamp.ReadAllLines();
return (
lines.Length > 0 ? lines[0].Trim() : string.Empty,
lines.Length > 1 ? lines[1].Trim() : string.Empty);
}
ReBuildTool/ReBuildTool.Service/PackageService/Fetchers/HttpArchivePackageFetcher.cs:48
- The “already unpacked” fast path ignores the manifest’s strip setting. If a user changes "strip" in RBTPackage.json, restore will still treat the existing directory as satisfied (by sha/url only) and will not re-extract with the new layout.
This issue also appears in the following locations of the same file:
- line 99
- line 108
var satisfied = expected != null
? Hashing.Matches(expected, stampedHash)
: stampedUrl == url;
if (satisfied)
ReBuildTool/ReBuildTool.Service/PackageService/PackageManifest.cs:145
- PinKey() ignores fields that materially change the restored result (e.g. archive "strip" and dependency "overlay"). That makes (a) conflicting declarations with different overlays/strip look interchangeable, and (b) lock entries potentially reused even when the manifest semantics changed.
return ResolveKind(packageName) switch
{
PackageSourceKind.Git => $"git:{Git}@{GitRevision}",
PackageSourceKind.HttpArchive => $"url:{Url}@{Sha256}",
PackageSourceKind.Path => $"path:{Path}",
Sample/PackageConsumer/global.json:7
- This global.json allows prerelease SDKs and rolls forward to the latest major. That can make builds non-reproducible over time (e.g. a future .NET 9 preview getting selected) even though the file pins 8.0.0.
给 rbt 加一套包管理：声明式清单 + lock 文件 + 传递依赖解析 + 四种来源 + 三种包形态。
为什么是这个设计
一个顺序约束决定了整体结构：
CppBuildProject.InitAllRule()用Assembly.LoadFile加载CompileRules.dll，一次性且不可卸载。所以包里的.module.cs必须在ParseRules()glob 之前就落到磁盘上——不存在「先编译规则、再发现依赖、再补进同一个程序集」这条路。由此：Parse()里、ParseRules()之前；这也解释了
CppTargetRule.GitLibraries为何从来没工作过：它挂在 target rule 上，只有规则程序集编译完才读得到。已标[Obsolete]并在文档中说明迁移方式。内容
RBTPackage.json+RBTPackage.lock.json（记录 tag 实际解析到的 commit，应当提交）overlayoverrides出口--Mode Restore、--Offline、--ForceRestore、--UpdateLock、--PackageAdd、--PackageRemove{ "dependencies": { "GreeterLib": { "git": "https://github.com/x/greeter.git", "tag": "v1.2.0" }, "zlib": { "url": "https://.../zlib-1.3.tar.gz", "sha256": "…", "strip": 1 }, "LocalLib": { "path": "../LocalLib" }, "fmt": { "vcpkg": "fmt", "triplet": "x64-windows" } } }几个判断点
Packages/放项目根目录而非Intermedia/：后者会被Clean()清空，且CleanIfNeed()在 rbt 二进制更新时自行触发，放那里等于每次 rebuild 都重新下载全部依赖。.module.cs，而不是直接注册IModuleInterface：这样InitAllRule的ModuleRulePaths查找、SetupInternal生命周期、_API宏代码生成、四种 IDE 生成器和 HeaderTool 插件全部原样继续工作。生成的文件不内联当前平台，产物由PackageArtifactSelector在Setup时按buildContext选择——否则切一次--TargetPlatform文件内容就变，会连带重编全部规则。SourceDirectories/ExcludeFiles要解析到上游代码树。WriteIfChanged语义（沿用ModuleRule.GenCode.cs的既有先例），避免时间戳抖动触发无谓重编。RBTPackage.json的项目完全不受影响：不建Packages/、不写 lock、不动.gitignore。没有做的部分
~/.rbt/PackageCache）未实现。git 包直接 clone 进Packages/<name>并保留.git，更新走 fetch + checkout。这样更简单、不会出现多项目共享 checkout 互相打架，代价是多个项目用同一依赖会各自 clone 一份。属于纯优化，可后补。"triplet"。测试
新增 59 个测试（41 → 100），全部离线 —— CI 跑三个平台，网络依赖的测试就是不稳定源：
git init造真仓库，通过文件路径 clone；HttpListener提供测试自己压出来的包；Sample/PackageConsumer通过路径依赖消费Sample/GeometryPackage，已加入原有样例构建套件，在三个平台上真实编译链接。本地结果：97 通过、2 跳过（Windows-only）、1 失败 ——
TestHeaderToolCodegen。该失败已用git stash在改动前的原始树上复现，与本 PR 无关（既有问题）。顺带
修掉
Doc/ARCH.md中已过时的ReBuildTool.Ini条目（该项目已不存在）。EN + zh-CN 四份文档均已更新。Generated by Claude Code