Skip to content

Switch the global tool during migration, and target the kept package id - #576

Open
ChrisonSimtian wants to merge 8 commits into
Fallout-build:mainfrom
ChrisonSimtian:bugfix/tool-manifest-package-id
Open

Switch the global tool during migration, and target the kept package id#576
ChrisonSimtian wants to merge 8 commits into
Fallout-build:mainfrom
ChrisonSimtian:bugfix/tool-manifest-package-id

Conversation

@ChrisonSimtian

@ChrisonSimtian ChrisonSimtian commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #581. This branch is rebased onto it, so the diff below includes #581's commits until that merges. Review the last commit (a278be9b) for this PR's own change, and merge #581 first.

Makes fallout-migrate fix the whole tool-id problem, not just the repo half of it.

Point the migration at the id we kept

#581 settled on keeping Fallout.GlobalTool rather than moving to Fallout.GlobalTools, so the current id and the retired list swap places:

  • current: fallout.globaltool
  • retired: nuke.globaltool, fallout.cli, fallout.globaltools

A consumer already pinning fallout.globaltool now migrates to nothing. That is the point of keeping the id, and there is a spec for it.

Switch the machine's global tool

Rewriting .config/dotnet-tools.json only fixes the repo. A machine-wide install of a retired tool stays behind, and two packages providing the fallout command conflict — so the repo-only fix left the machine broken.

New SwitchGlobalToolStep reads dotnet tool list --global, uninstalls each retired id that is installed, then installs the current one at the resolved version.

  • Runs after RewriteToolManifestStep. Repo first, so an interrupted run still leaves the committed state correct.
  • Best-effort throughout. No dotnet on PATH, no global install, or an unreachable feed produces a Summary warning naming the command to run by hand. It never fails the migration.
  • --dry-run prints the commands without running them.
  • Does nothing when no retired id is installed, so the common case is silent.

Process execution

SwitchGlobalToolStep shells out through the framework's own ProcessTasks (src/Fallout.Tooling). No new package dependency.

CliWrap was trialled here first and rejected — findings, the side-by-side code, and the measured comparison are in #583. Short version: both versions behave identically, the call site is 31 lines instead of 18, and ProcessTasks already handles the pipe-buffer deadlock that makes hand-rolled process runners dangerous. The four places ProcessTasks is a worse API (no CancellationToken, no async overload, pre-quoted argument string, Serilog by default) are acceptance criteria on #583 rather than a reason to take the package.

Referencing Fallout.Tooling does pull Serilog and NuGet.Packaging into the migrate tool — one direct dependency traded for two transitive ones. Called out because Fallout.Migrate is otherwise deliberately lean.

Also

  • .config/dotnet-tools.json moves to fallout.globaltool 10.3.49. The dogfood pin could not move while the id was Fallout.GlobalTools, because nothing above 10.4.0-rc.4 was published there. Verified dotnet tool restore resolves it.
  • docs/migration/tool-package-id.md rewritten around the kept id. It is now a "move off a retired id" page, and most readers need no action.

Verification

  • Fallout.Migrate.Specs 63/63, including 8 new specs for the tool-list parsing and install-command rendering
  • A direct probe through ProcessTasks returns the same seven global tool ids the CliWrap version did
  • dotnet build fallout.slnx clean
  • fallout-migrate --dry-run against a scratch repo pinning fallout.cli rewrites the manifest as expected
  • dotnet tool restore resolves the new dogfood pin

Notes

🤖 Generated with Claude Code

@ChrisonSimtian ChrisonSimtian added bug Something isn't working target/vCurrent Targets the current version labels Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok something here went wrong @claude pretty sure you went a bit too far here

@ChrisonSimtian

Copy link
Copy Markdown
Collaborator Author

alternatively we rename this back to singular

@ChrisonSimtian ChrisonSimtian added breaking-change Change is breaking — requires major version bump per CLAUDE.md semver policy. Dont Merge This cant be merged for various reasons (i.e. its being hold back, its broken, etc) labels Jul 27, 2026
The dotnet-tool package id had been changed to Fallout.GlobalTools (plural)
ahead of first release. Revert it: Fallout.GlobalTool is the id the entire
10.2.x/10.3.x install base already has pinned (52 published versions, latest
10.3.49), so continuing to publish under it means nobody migrates and no id
has to be unlisted. Assembly/namespace stay Fallout.Cli; command stays
`fallout`.

- Fallout.Cli.csproj: <PackageId>Fallout.GlobalTool</PackageId>, with a comment
  recording why it deliberately doesn't match the assembly name.
- templates/dotnet-tools.json + BuildScaffolder comments: `fallout :setup`
  writes a `fallout.globaltool` pin into consumer repos again.
- UpdateNotificationAttribute: in-tool prompt says `dotnet tool update
  Fallout.GlobalTool -g`.
- build/Build.cs: the Install target installed `Solution.Fallout_Cli.Name`
  ("Fallout.Cli"), which never matched the packed nupkg id. Point it at a new
  GlobalToolPackageId constant instead.

Docs swept repo-wide for the plural id: README install + note, CONTRIBUTING,
introduction, 01-installation, 02-setup, 03-execution, github-actions,
rebrand-plan. docs/migration/from-globaltool-to-cli.md deleted — it documented
a GlobalTool -> Cli rename that is now explicitly never happening, and was
already draft:true with no inbound links.

Left alone: .config/dotnet-tools.json still pins fallout.cli 10.3.45. That is
the repo's own dogfood pin and flipping it needs a published Fallout.GlobalTool
build newer than 10.3.49; doing it here would regress `dotnet tool restore`.

Verified: pack produces Fallout.GlobalTool.*.nupkg; Fallout.Cli.Specs 47/47.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback on Fallout-build#581: the Install target shouldn't carry yet another
hardcoded copy of the package id. Fallout.ProjectModel already exposes MSBuild
property access on the solution model, so walk the object path instead:

    Solution.Fallout_Cli.GetProperty("PackageId")   // -> Fallout.GlobalTool

Drops the GlobalToolPackageId constant added in the previous commit. The csproj
is now the single source of truth for the id, so changing it later touches one
file.

No new API was needed — ProjectExtensions.GetProperty (src/Fallout.ProjectModel)
has covered this since the NUKE days, and the same MSBuild-evaluation path is
already exercised by this build at Build.cs:98 via GetTargetFrameworks().

Verified end-to-end against fallout.slnx: GetProperty("PackageId") on the
Fallout.Cli project returns "Fallout.GlobalTool".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChrisonSimtian and others added 3 commits July 28, 2026 10:09
fallout-migrate had no handling for dotnet-tools.json at all, so a consumer
pinning a retired tool id kept that pin through a migration and stayed stuck on
the last version published under it.

Add RewriteToolManifestStep. It renames nuke.globaltool, fallout.globaltool and
fallout.cli to fallout.globaltools, and re-pins the version of the entries it
renames. The rename alone is not enough: the pinned version belongs to the old
id and does not exist under the new one, so the manifest would stop restoring.

The tool version is resolved separately from MigrationContext.FalloutVersion.
The tool ships under its own package id, introduced later, so it has a different
set of published versions — fallout.globaltools currently has only a prerelease.
ResolveFalloutVersionStep now prefers the latest stable within the major and
falls back to the newest prerelease, and RewriteToolManifestStep warns when it
pinned a prerelease or could not resolve one. The warning only fires when a
manifest was actually rewritten.

Verified end to end against a copy of a real stuck manifest: the migrated
manifest restores fallout.globaltools 10.4.0-rc.4.

Part of Fallout-build#575

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- docs/introduction.md installed Fallout.GlobalTool on one line and
  Fallout.GlobalTools 27 lines later.
- docs/migration/from-globaltool-to-cli.md described the Fallout.Cli rename,
  which never shipped. It was already draft:true and superseded, and its facts
  were wrong: it claimed Fallout.GlobalTool was last published at 10.3.40 and
  unlisted, when it is listed with 52 versions up to 10.3.49. Replaced with
  docs/migration/tool-package-id.md, covering the rename that did ship.
- README's upgrade note now says the version pin has to change too, and links
  the guide.
- This repo pinned fallout.cli 10.3.45, an unlisted version of a retired id, so
  it was not dogfooding its own published tool. Flipped to fallout.globaltools.
  Verified ./build.sh still runs the engine and leaves the tree clean.

Part of Fallout-build#575

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t id

Two changes to what fallout-migrate does about the tool package id.

Point the migration at Fallout.GlobalTool. Fallout-build#581 settled on keeping the
NUKE-era id rather than moving to Fallout.GlobalTools, so the current id and
the retired list swap places:

  current:  fallout.globaltool
  retired:  nuke.globaltool, fallout.cli, fallout.globaltools

A consumer already pinning fallout.globaltool now migrates to nothing, which is
the point of keeping that id. Covered by a spec.

Add SwitchGlobalToolStep. Rewriting .config/dotnet-tools.json only fixes the
repo. A machine-wide install of a retired tool stays behind, and two packages
providing the `fallout` command conflict, so the repo-only fix left the machine
broken. The new step reads `dotnet tool list --global`, uninstalls each retired
id that is installed, then installs the current one at the resolved version.

It runs after RewriteToolManifestStep: repo first, so an interrupted run still
leaves the committed state correct. Every command is best-effort — no dotnet on
PATH, no global install, or an unreachable feed produces a Summary warning
naming the command to run by hand, never a failed migration. --dry-run prints
the commands without running them.

Introduces CliWrap (3.10.3, central pin) for the process calls: buffered output
and a cancellation token without hand-rolled ProcessStartInfo plumbing. Scoped
to Fallout.Migrate on purpose. Fallout.Common has its own ProcessTasks layer
that build authors consume, and that is not changing. Added to
docs/dependencies.md under a new Console / process section.

Also flips .config/dotnet-tools.json to fallout.globaltool 10.3.49. The pin
could not move while the id was Fallout.GlobalTools, because nothing was
published there above 10.4.0-rc.4. Verified: `dotnet tool restore` resolves it.

docs/migration/tool-package-id.md rewritten around the kept id. It is now a
"move off a retired id" page, and most readers need no action at all.

Verified: Fallout.Migrate.Specs 63/63; solution builds; dry-run against a
scratch repo pinning fallout.cli rewrites the manifest as expected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ChrisonSimtian
ChrisonSimtian force-pushed the bugfix/tool-manifest-package-id branch from 0016983 to a278be9 Compare July 27, 2026 22:15
@ChrisonSimtian ChrisonSimtian changed the title Migrate dotnet-tools.json off the retired Fallout tool ids Switch the global tool during migration, and target the kept package id Jul 27, 2026
/// shorter, set of published versions (#575).
/// </summary>
private static readonly Uri flatContainerIndex = new($"https://api.nuget.org/v3-flatcontainer/{PackageId}/index.json");
private const string ToolPackageId = RewriteToolManifestStep.CurrentToolId;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude same as in #581 you can point this right at the PackageId property of the Fallout Cli project, instead of hardcoding a string value here. This way we always resolve the correct package!

[
"nuke.globaltool",
"fallout.globaltools",
"fallout.cli"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a brief moment in time where this package did exist. Lets leave this in here just in case

ChrisonSimtian and others added 2 commits July 28, 2026 10:40
CliWrap was trialled for SwitchGlobalToolStep's `dotnet tool` calls and is
rejected. Fallout has its own process layer, so the migrate tool should use it
rather than add a third-party process runner. Findings recorded in Fallout-build#583.

Both versions were built and run side by side against `dotnet tool list
--global`, a failing uninstall, and a 1ms timeout: identical exit codes,
identical stdout, identical stderr, both null on timeout. The measured cost of
dropping CliWrap is 18 lines of call-site code against 31, at one call site.

- Directory.Packages.props / Fallout.Migrate.csproj: drop the CliWrap pin and
  reference Fallout.Tooling instead.
- SwitchGlobalToolStep: Run() now goes through ProcessTasks.StartProcess with
  logging off, since this tool renders through Spectre and ProcessTasks logs
  through Serilog. ProcessTasks is synchronous, so the step's work moved into a
  private Execute() and ExecuteAsync() is a Task.CompletedTask wrapper — the
  same shape RewriteToolManifestStep already uses.
- Local CommandResult record replaces BufferedCommandResult.
- docs/dependencies.md: no CliWrap row; records that process execution is
  served in-house and points at Fallout-build#583.

ProcessTasks is a worse API for this than CliWrap on four counts — no
CancellationToken, no async overload, arguments as one pre-quoted string, and
Serilog-by-default. Fallout-build#583 carries those as acceptance criteria rather than
routing around them with a package.

Verified: Fallout.Migrate.Specs 63/63; solution builds; `fallout-migrate
--dry-run` against a scratch repo pinning fallout.cli rewrites the manifest and
reports no warnings, meaning the ProcessTasks-backed `dotnet tool list --global`
call ran and parsed; a direct probe through ProcessTasks returns the same seven
tool ids CliWrap did.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI on this branch went into a loop of repeated `dotnet tool restore` and was
killed by the OOM killer after 11 minutes. Two problems, one of which is a real
bug in the previous commits.

Revert .config/dotnet-tools.json to fallout.cli 10.3.45. Moving it to
fallout.globaltool 10.3.49 was verified only as far as `dotnet tool restore`
succeeding, which it does. What was never checked is that a 10.3.49 CLI can
actually run this repo's build, and CI says something in that path does not
terminate. 10.3.49 is a late one-off publish under the old id, not a build this
branch has ever run against. The pin stays where main has it until a
Fallout.GlobalTool release newer than the 10.4 line exists — which is the
constraint Fallout-build#581 documented and this branch wrongly assumed it had cleared.

Gate SwitchGlobalToolStep behind --switch-global-tool, default off.
MigrationIntegrationSpecs runs the real pipeline with dryRun: false, so the step
was shelling out to `dotnet tool list --global` on every spec run, and would
have run uninstall/install on any machine that happened to have a retired id
installed. A unit test run must not change what software is on the machine.
Beyond the specs, silently changing a machine-wide install is too large a side
effect for a tool whose job is to migrate a repository, so opt-in is the right
default regardless of the test problem.

Flag threads MigrateSettings -> Migration -> MigrationContext, next to DryRun.
docs/migration/tool-package-id.md documents it.

Verified: Fallout.Migrate.Specs 63/63. CI is the real check for the pin revert.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ChrisonSimtian ChrisonSimtian removed breaking-change Change is breaking — requires major version bump per CLAUDE.md semver policy. Dont Merge This cant be merged for various reasons (i.e. its being hold back, its broken, etc) labels Jul 27, 2026
@ChrisonSimtian
ChrisonSimtian marked this pull request as ready for review July 27, 2026 23:37
@ChrisonSimtian
ChrisonSimtian requested a review from a team as a code owner July 27, 2026 23:37
@ChrisonSimtian

Copy link
Copy Markdown
Collaborator Author

alright, this got some smarts now:
It ALWAYS runs the migration, it has a list of retired variants that we replace and it goes and makes sure the user has the CLI tool installed as per the Cli-Project's PackageId. This means should we change the PackageID in the future, the migration will automatically(!!) pick it up and migrate it for the user

Comment on lines +42 to +51
Execute(context, summary);

return Task.CompletedTask;
}

/// <summary>
/// The step's actual work. Synchronous because <see cref="ProcessTasks"/> is; the interface's
/// async signature is satisfied by the wrapper above.
/// </summary>
private static void Execute(MigrationContext context, Summary summary)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the big advantage of "just" forwarding to a private method with the exact same parameters?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats a @claude special. Go forth AI and fix your own slop

ChrisonSimtian added a commit to ChrisonSimtian/Fallout that referenced this pull request Jul 28, 2026
The tool id the migration moves consumers onto was a literal, independent of
the id Fallout.Cli is actually published under. The two could drift, and a
migration would then pin an id that does not exist.

Holds the id in one place instead: a FalloutGlobalToolPackageId property in
Directory.Build.props feeds Fallout.Cli's PackageId, and a small MSBuild
target compiles the same value into Fallout.Migrate as a constant. Lowercased
on the way in, so written dotnet-tools.json manifests stay byte-identical to
what the previous literal produced.

Runtime lookup is not an option here: fallout-migrate runs against a
consumer's repository, where no Fallout csproj exists, so the
Solution.Fallout_Cli.GetProperty("PackageId") approach used in build/Build.cs
cannot reach anything.

Both projects fail the build if the property is ever empty. Without that,
Fallout.Cli would quietly fall back to its assembly name and publish as
Fallout.Cli, which is the id Fallout-build#581 exists to avoid.

This is a hotfix so Fallout-build#576 is not blocked. Fallout-build#584 designs the general mechanism
for flowing published ids into shipped code and should replace all of it; the
three touched files carry HOTFIX notes saying what to remove.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ChrisonSimtian
ChrisonSimtian force-pushed the bugfix/tool-manifest-package-id branch from 99c677d to e5e3de5 Compare July 28, 2026 09:57
ExecuteAsync forwarded to a private Execute taking the same parameters. The
only thing that bought was letting the early-exit guards stay as bare
`return;` instead of `return Task.CompletedTask;`.

Every other synchronous step in Steps/ puts its body straight into
ExecuteAsync, RenameNukeDirectoryStep and RewriteToolManifestStep included,
and both repeat `return Task.CompletedTask;`. This step was the only one
shaped differently, for no gain.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working target/vCurrent Targets the current version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants