feat(packaging): three-crate facade — cargo install shipper (#95)#123
feat(packaging): three-crate facade — cargo install shipper (#95)#123EffortlessSteven wants to merge 2 commits into
Conversation
Prep commit for #95 three-crate facade. Zero behavior change: - `src/main.rs` → `src/lib.rs`; `fn main()` → `pub fn run()`. - new `src/main.rs` is a 3-line shim: `shipper_cli::run()`. - lib-level doc comment notes the façade crate's role as the install entry point. The seam exists so the upcoming `crates/shipper` façade crate can own the `[[bin]] name = "shipper"` slot and call `shipper_cli::run()` without duplicating CLI logic.
…cade (#95) Part 2 of the three-crate facade layout for Shipper. This commit physically moves the monolith library into `crates/shipper-core` and introduces a new UX-focused `crates/shipper` crate whose only job is to be the 'install me' entry point. ## New crate layout - `crates/shipper-core` — library (renamed from `crates/shipper`, `package.name = "shipper-core"`). All real code lives here. Published as `shipper-core` on crates.io. - `crates/shipper-cli` — unchanged conceptually. Its binary target is renamed from `[[bin]] name = "shipper"` to `shipper-cli` so the facade can own the `shipper` binary name. `pub fn run()` remains the sole public entry point (prep from the prior commit). - `crates/shipper` — new facade. `[[bin]] name = "shipper"` with a three-line `main.rs` that calls `shipper_cli::run()`. `src/lib.rs` is `pub use shipper_core::*;` so library users doing `shipper = \"0.3\"` get exactly the same public API. Published as `shipper` — `cargo install shipper` works. ## Mechanical changes - `git mv crates/shipper crates/shipper-core` preserves blame. - Workspace `Cargo.toml` adds `crates/shipper-core` to `members` and `workspace.dependencies`. - `shipper-cli/Cargo.toml` aliases the core dep as `shipper` (`shipper = { package = "shipper-core", path = "../shipper-core" }`) so existing `use shipper::...` imports inside the CLI and its tests continue to work verbatim. `workspace = true` can't combine with `package = "..."`, so path + version are explicit. - `fuzz/Cargo.toml` uses the same alias trick so fuzz targets stay unchanged. - `shipper-core/tests/**/*.rs` rewrote `shipper::` → `shipper_core::` (integration tests are a separate compilation unit and don't see Cargo's dep alias). - `shipper-cli/tests/**/*.rs` rewrote `cargo_bin!("shipper")` → `cargo_bin!("shipper-cli")` since the CLI crate's bin renamed. - Insta snapshot filenames rebaselined throughout `shipper-core/src/**/snapshots/` (module_path went from `shipper::...` to `shipper_core::...`). Orphans deleted. - `.github/workflows/architecture-guard.yml` paths updated to `crates/shipper-core/src/**`. - `.github/workflows/release.yml` switches every `cargo build -p shipper-cli` to `-p shipper` so the facade's `shipper` binary is what gets installed to `/usr/local/bin`. CRATES publish list adds `shipper-core` and reorders for topo publish order (core → cli → facade). ## User-facing impact - `cargo install shipper --locked` now installs the `shipper` binary directly (operator UX goal of #95). - Library consumers (`shipper = \"0.3\"`) get the same public API unchanged, re-exported verbatim from `shipper-core`. - Operators who prefer pinning to the CLI crate directly can still `cargo install shipper-cli --locked` and get a `shipper-cli` binary on PATH. Closes #95.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (190)
📒 Files selected for processing (110)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors the workspace by renaming the core logic crate to shipper-core and moving the CLI implementation into a library module within shipper-cli. The shipper facade is updated to point to these new components, and documentation and test snapshots are refreshed to reflect the new structure. Feedback identifies a compilation error in match arms involving temporary strings, recommends replacing hardcoded ANSI escape sequences with the console crate for better terminal compatibility, and suggests updating documentation paths and examples to align with the crate renames. Additionally, a suggestion was made to avoid hardcoded versions in dependency aliases to simplify future maintenance.
| let state_str = match &p.state { | ||
| shipper::types::PackageState::Published => "\x1b[32mPublished\x1b[0m", | ||
| shipper::types::PackageState::Pending => "Pending", | ||
| shipper::types::PackageState::Uploaded => "\x1b[33mUploaded\x1b[0m", | ||
| shipper::types::PackageState::Skipped { reason } => &format!("Skipped: {}", reason), | ||
| shipper::types::PackageState::Failed { class, message } => { | ||
| &format!("\x1b[31mFailed ({:?}): {}\x1b[0m", class, message) | ||
| } | ||
| shipper::types::PackageState::Ambiguous { message } => { | ||
| &format!("\x1b[33mAmbiguous: {}\x1b[0m", message) | ||
| } | ||
| }; |
There was a problem hiding this comment.
This match statement has inconsistent return types in its arms and attempts to return a reference to a temporary value, which will not compile. Some arms return a &'static str, while others attempt to return a &String where the String is a temporary created by format!.
To fix this, all arms should return an owned String. This also provides an opportunity to replace the hardcoded ANSI color codes with the console crate.
| let state_str = match &p.state { | |
| shipper::types::PackageState::Published => "\x1b[32mPublished\x1b[0m", | |
| shipper::types::PackageState::Pending => "Pending", | |
| shipper::types::PackageState::Uploaded => "\x1b[33mUploaded\x1b[0m", | |
| shipper::types::PackageState::Skipped { reason } => &format!("Skipped: {}", reason), | |
| shipper::types::PackageState::Failed { class, message } => { | |
| &format!("\x1b[31mFailed ({:?}): {}\x1b[0m", class, message) | |
| } | |
| shipper::types::PackageState::Ambiguous { message } => { | |
| &format!("\x1b[33mAmbiguous: {}\x1b[0m", message) | |
| } | |
| }; | |
| let state_str = match &p.state { | |
| shipper::types::PackageState::Published => console::style("Published").green().to_string(), | |
| shipper::types::PackageState::Pending => "Pending".to_string(), | |
| shipper::types::PackageState::Uploaded => console::style("Uploaded").yellow().to_string(), | |
| shipper::types::PackageState::Skipped { reason } => format!("Skipped: {}", reason), | |
| shipper::types::PackageState::Failed { class, message } => { | |
| console::style(format!("Failed ({:?}): {}", class, message)).red().to_string() | |
| } | |
| shipper::types::PackageState::Ambiguous { message } => { | |
| console::style(format!("Ambiguous: {}", message)).yellow().to_string() | |
| } | |
| }; |
| - Root cause of past failures: grep matched forbidden-import examples inside `crates/shipper/src/ops/CLAUDE.md` (the CLAUDE.md documents the rule with `❌ use crate::engine::...` lines, which the guard scanned as real violations). | ||
| - **Fix merged in #85 (`ea75b53`):** added `--include='*.rs'` to every guard grep. Current `.github/workflows/architecture-guard.yml` on main is correct. | ||
| - **Why it still shows red in the runs list:** the guard's `paths:` filter restricts triggering to `crates/shipper/src/**`. No commit on main since #85 has touched that path — #86 only touched `crates/shipper-cli/tests/`. So the fixed guard has not had an opportunity to re-run and post a green status. | ||
| - **Risk:** low-confidence green. The fix is in place but unverified until the next commit that touches `crates/shipper/src/**` triggers it. |
There was a problem hiding this comment.
The file paths mentioned in this document seem to be outdated after the shipper to shipper-core rename. For example, crates/shipper/src/ops/CLAUDE.md should likely be crates/shipper-core/src/ops/CLAUDE.md, and paths like crates/shipper/src/** should be updated to crates/shipper-core/src/** in this section and on line 138.
| # after the upstream rename to `shipper-core` (#95). `workspace = true` | ||
| # can't combine with `package = "..."`, so pin path + version | ||
| # explicitly. Library API is re-exported unchanged. | ||
| shipper = { package = "shipper-core", path = "../shipper-core", version = "0.3.0-rc.1" } |
There was a problem hiding this comment.
The hardcoded version 0.3.0-rc.1 for shipper-core could become a maintenance issue. When the workspace version is bumped, this will need to be updated manually here and in other places using this aliasing trick (like fuzz/Cargo.toml).
While aliasing with workspace = true isn't possible, a future refactoring could remove the alias and use shipper-core directly throughout this crate. This would allow you to use shipper-core.workspace = true and inherit the version from the workspace definition, removing the hardcoded version string.
| #[allow(clippy::collapsible_if)] | ||
| if let Some(deps) = ws.plan.dependencies.get(&p.name) { | ||
| if deps.len() > 3 { | ||
| issues.push(format!( | ||
| " - {}@{} has {} dependencies (may require longer publish time)", | ||
| p.name, | ||
| p.version, | ||
| deps.len() | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
The collapsible_if lint is suppressed here, but the if statements can be combined. This improves readability by reducing nesting.
if let Some(deps) = ws.plan.dependencies.get(&p.name) && deps.len() > 3 {
issues.push(format!(
" - {}@{} has {} dependencies (may require longer publish time)",
p.name,
p.version,
deps.len()
));
}| let (finishability_color, finishability_text) = match rep.finishability { | ||
| Finishability::Proven => ("\x1b[32m", "PROVEN"), | ||
| Finishability::NotProven => ("\x1b[33m", "NOT PROVEN"), | ||
| Finishability::Failed => ("\x1b[31m", "FAILED"), | ||
| }; | ||
| println!( | ||
| "Finishability: {}{}", | ||
| finishability_color, finishability_text | ||
| ); |
There was a problem hiding this comment.
Hardcoded ANSI escape codes are used for coloring output. This can lead to issues on terminals that don't support colors or when output is redirected to a file. It also doesn't respect the NO_COLOR environment variable.
Since the console crate is already a dependency, it's better to use it for styling text. This will handle color support automatically.
This also applies to other parts of this function that use hardcoded colors (e.g., lines 939-952).
| let (finishability_color, finishability_text) = match rep.finishability { | |
| Finishability::Proven => ("\x1b[32m", "PROVEN"), | |
| Finishability::NotProven => ("\x1b[33m", "NOT PROVEN"), | |
| Finishability::Failed => ("\x1b[31m", "FAILED"), | |
| }; | |
| println!( | |
| "Finishability: {}{}", | |
| finishability_color, finishability_text | |
| ); | |
| let finishability_styled = match rep.finishability { | |
| Finishability::Proven => console::style("PROVEN").green(), | |
| Finishability::NotProven => console::style("NOT PROVEN").yellow(), | |
| Finishability::Failed => console::style("FAILED").red(), | |
| }; | |
| println!("Finishability: {}", finishability_styled); |
| use shipper::config::{CliOverrides, ShipperConfig}; | ||
| use shipper::engine::{self, Reporter}; | ||
| use shipper::plan; | ||
| use shipper::types::{Registry, ReleaseSpec}; |
There was a problem hiding this comment.
Since this README is for the shipper-core crate, the example should probably use shipper_core in the use statements to reflect how it would be used when depended upon directly. A user might depend on shipper-core to avoid the dependencies of the shipper facade.
| use shipper::config::{CliOverrides, ShipperConfig}; | |
| use shipper::engine::{self, Reporter}; | |
| use shipper::plan; | |
| use shipper::types::{Registry, ReleaseSpec}; | |
| use shipper_core::config::{CliOverrides, ShipperConfig}; | |
| use shipper_core::engine::{self, Reporter}; | |
| use shipper_core::plan; | |
| use shipper_core::types::{Registry, ReleaseSpec}; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90cec907c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sha2 = "0.11" | ||
| [dependencies] | ||
| shipper-core.workspace = true | ||
| shipper-cli.workspace = true |
There was a problem hiding this comment.
Make
shipper-cli optional for library consumers
The new façade crate declares shipper-cli as an unconditional dependency even though crates/shipper/src/lib.rs only re-exports shipper_core::* and never uses CLI code. This means every downstream project that depends on shipper as a library now has to compile the full CLI stack (clap, progress UI, etc.), which is a measurable build-time/perf regression and increases dependency-surface risk for non-CLI use cases. Please gate shipper-cli behind a bin-only feature (or equivalent) so library consumers only pull shipper-core.
Useful? React with 👍 / 👎.
|
Parking this PR as draft per sequencing feedback: three-crate facade was the wrong shape for #95, and #95 itself must wait until #97 (rehearsal-registry proof) and #98 (remediation) land first. The correct #95 direction is narrower: keep Will revisit after #97 + #98 land and #95 issue text is rewritten to the narrower shape. |
#95 reframe — narrower shape (captured)After session review, the three-crate facade shape in this PR was the wrong direction. Parking it here as a reference implementation; the real #95 should be rewritten from the narrower shape below. Keeping this comment on the PR so future work starts from the corrected premise instead of re-deriving it. The narrower shape
Package-level unification, architectural separation preserved. Why this is better than the three-crate facade
SequencingStill after #97 and #98 land. The issue has always been packaging polish, not a prerequisite — and the remediation pillar (#98) + prove tier 2 (#97) are the remaining release-closure gaps that need to close first. Implementation sketch when the time comes
What this PR stays useful for
Leaving this as draft indefinitely. Close when the reframed #95 lands. |
Reframed packaging UX. Supersedes the parked three-crate facade (#123). ## What changes - **\`shipper\`** becomes a single public crate with both \`[lib]\` and \`[[bin]]\`. The CLI lives at \`shipper::cli\` (module, pub) and the binary target at \`src/bin/shipper.rs\` is a three-line shim over \`shipper::cli::run()\`. - **\`shipper-cli\`** stays published as a compatibility shim: \`[[bin]] name = \"shipper-cli\"\` that forwards into \`shipper::cli::run()\`. Operators on the old name keep working; new setups should \`cargo install shipper --locked\`. - README, docs, release workflow updated to prefer \`cargo install shipper\` everywhere. ## Why this shape - **One public product story.** Library users \`shipper = \"0.3\"\`, operators \`cargo install shipper\`. One crate to learn, document, cite. - **No dep cycles.** The earlier three-crate \`shipper-core\` + \`shipper\` + \`shipper-cli\` facade kept tripping cycle risk because the facade depended on the CLI which depended on the library. A single crate with a \`cli\` submodule sidesteps this. - **No hidden core.** \`shipper-core\` as a publicly meaningful crate would have forced consumers to choose between \`shipper\` and \`shipper-core\` for no gain. - **Smaller churn than the three-crate attempt.** One directory move (\`shipper-cli/src/\` → \`shipper/src/cli/\`) + bin rename, vs the ~570-file shuffle the earlier attempt produced. ## Mechanical changes - \`shipper-cli/src/main.rs\` + \`shipper-cli/src/output/\` moved under \`shipper/src/cli/\` (git mv preserves history). \`fn main\` became \`pub fn run\`. All \`use shipper::...\` rewrote to \`use crate::...\`. - \`shipper/Cargo.toml\` gains \`[[bin]] name = \"shipper\"\` + the CLI's deps (clap, clap_complete, indicatif). - \`shipper-cli/Cargo.toml\` reduces to a shim: \`[[bin]] name = \"shipper-cli\"\`, \`src/main.rs\` is three lines. - \`shipper-cli/tests/\` preserved (25 files, extensive coverage). Every \`cargo_bin!(\"shipper\")\` → \`cargo_bin!(\"shipper-cli\")\`. Tests now exercise the shim, which internally calls the same \`shipper::cli::run\` — so functional coverage is identical. - Help-text snapshots regenerated: \`Usage: shipper-cli\` when invoked via the shim (correct behavior — it IS shipper-cli when you run it that way). \`cli::output::progress\` snapshots moved to their new module path under \`shipper/src/cli/\`. - Workspace \`default-members\` reordered (\`shipper\` first). - Release workflow builds \`-p shipper\` instead of \`-p shipper-cli\`, installs \`target/release/shipper\` to PATH (facade bin name). - Docs (README, release-runbook, run-in-github-actions, tutorials, failure-modes, preflight, tech) all switch to \`cargo install shipper --locked\`. ## Acceptance - [x] \`cargo install --path crates/shipper --locked\` installs a working \`shipper\` binary. - [x] \`cargo install --path crates/shipper-cli --locked\` still works (produces a \`shipper-cli\` binary via the shim). - [x] \`shipper --version\` and \`shipper-cli --version\` both print the version. - [x] Library imports (\`use shipper::engine;\` etc.) unchanged. - [x] \`cargo clippy --workspace --all-targets --all-features -- -D warnings\` clean. - [x] \`cargo doc -p shipper --no-deps\` clean. - [x] \`cargo fmt --all --check\` clean.
* feat(packaging): unify shipper CLI into the shipper crate (#95) Reframed packaging UX. Supersedes the parked three-crate facade (#123). ## What changes - **\`shipper\`** becomes a single public crate with both \`[lib]\` and \`[[bin]]\`. The CLI lives at \`shipper::cli\` (module, pub) and the binary target at \`src/bin/shipper.rs\` is a three-line shim over \`shipper::cli::run()\`. - **\`shipper-cli\`** stays published as a compatibility shim: \`[[bin]] name = \"shipper-cli\"\` that forwards into \`shipper::cli::run()\`. Operators on the old name keep working; new setups should \`cargo install shipper --locked\`. - README, docs, release workflow updated to prefer \`cargo install shipper\` everywhere. ## Why this shape - **One public product story.** Library users \`shipper = \"0.3\"\`, operators \`cargo install shipper\`. One crate to learn, document, cite. - **No dep cycles.** The earlier three-crate \`shipper-core\` + \`shipper\` + \`shipper-cli\` facade kept tripping cycle risk because the facade depended on the CLI which depended on the library. A single crate with a \`cli\` submodule sidesteps this. - **No hidden core.** \`shipper-core\` as a publicly meaningful crate would have forced consumers to choose between \`shipper\` and \`shipper-core\` for no gain. - **Smaller churn than the three-crate attempt.** One directory move (\`shipper-cli/src/\` → \`shipper/src/cli/\`) + bin rename, vs the ~570-file shuffle the earlier attempt produced. ## Mechanical changes - \`shipper-cli/src/main.rs\` + \`shipper-cli/src/output/\` moved under \`shipper/src/cli/\` (git mv preserves history). \`fn main\` became \`pub fn run\`. All \`use shipper::...\` rewrote to \`use crate::...\`. - \`shipper/Cargo.toml\` gains \`[[bin]] name = \"shipper\"\` + the CLI's deps (clap, clap_complete, indicatif). - \`shipper-cli/Cargo.toml\` reduces to a shim: \`[[bin]] name = \"shipper-cli\"\`, \`src/main.rs\` is three lines. - \`shipper-cli/tests/\` preserved (25 files, extensive coverage). Every \`cargo_bin!(\"shipper\")\` → \`cargo_bin!(\"shipper-cli\")\`. Tests now exercise the shim, which internally calls the same \`shipper::cli::run\` — so functional coverage is identical. - Help-text snapshots regenerated: \`Usage: shipper-cli\` when invoked via the shim (correct behavior — it IS shipper-cli when you run it that way). \`cli::output::progress\` snapshots moved to their new module path under \`shipper/src/cli/\`. - Workspace \`default-members\` reordered (\`shipper\` first). - Release workflow builds \`-p shipper\` instead of \`-p shipper-cli\`, installs \`target/release/shipper\` to PATH (facade bin name). - Docs (README, release-runbook, run-in-github-actions, tutorials, failure-modes, preflight, tech) all switch to \`cargo install shipper --locked\`. ## Acceptance - [x] \`cargo install --path crates/shipper --locked\` installs a working \`shipper\` binary. - [x] \`cargo install --path crates/shipper-cli --locked\` still works (produces a \`shipper-cli\` binary via the shim). - [x] \`shipper --version\` and \`shipper-cli --version\` both print the version. - [x] Library imports (\`use shipper::engine;\` etc.) unchanged. - [x] \`cargo clippy --workspace --all-targets --all-features -- -D warnings\` clean. - [x] \`cargo doc -p shipper --no-deps\` clean. - [x] \`cargo fmt --all --check\` clean. * test: strip .exe from shipper-cli binary name in help snapshots Fixes CI failures on Ubuntu + macOS where the bin has no .exe suffix. The normalize_output / normalize_stderr helpers were stripping `shipper.exe` but not `shipper-cli.exe`, so my Windows-regenerated snapshots leaked the platform-specific suffix. Order matters in the rewrite: strip `shipper-cli.exe` → `shipper-cli` first, else the subsequent `shipper.exe` → `shipper` rule would eat the `-cli` suffix after trimming `.exe`.
Summary
Close #95 by landing the three-crate façade layout for Shipper:
shipper-core— library (renamed fromcrates/shipper). Allreal publishing logic lives here. Published as
shipper-coreoncrates.io.
shipper-cli— CLI implementation (unchanged conceptually).Its binary is renamed from
[[bin]] name = "shipper"toshipper-cliso the façade can own theshippername. Exposespub fn run()as its single public entry point.shipper— NEW UX façade.[[bin]] name = "shipper"with athree-line
main.rscallingshipper_cli::run();src/lib.rsispub use shipper_core::*;. Published asshipper— operatorscan finally run
cargo install shipper --locked.User-facing goal achieved:
cargo install shipper --locked # was: cargo install shipper-cli shipper planLibrary API is unchanged (
shipper = "0.3"→ same re-exports).Commits
refactor(shipper-cli): split main.rs into lib.rs + thin main.rs—prep commit; zero behavior change.
refactor: rename crates/shipper -> crates/shipper-core + introduce facade—the actual rename + facade creation + CI/doc/snapshot updates.
Dep alias pattern
shipper-cli/Cargo.tomlaliases the core dep asshipper:This keeps
use shipper::…insideshipper-cli/src/lib.rsand itsintegration tests working verbatim. Same trick in
fuzz/Cargo.toml.shipper-core's own integration tests live in the crate itself andsaw
shipper::→shipper_core::rewrites (they're a separatecompilation unit and don't see Cargo's dep alias).
CI adjustments
.github/workflows/architecture-guard.yml: pathscrates/shipper/src/**→
crates/shipper-core/src/**(10 sites)..github/workflows/release.yml: builds-p shipper(façade) insteadof
-p shipper-cli, so the installed binary is the façade'sshipperbin that callsshipper_cli::run(). CRATES publish listadds
shipper-coreand orders topologically: core → cli → facade.Snapshot rebaseline
~330 files in
crates/shipper-core/src/**/snapshots/renamed fromshipper__…→shipper_core__…(insta usesmodule_path!()whichfollows the crate name). Orphans deleted; no test semantics changed.
Test plan
cargo build --workspace— clean.cargo test -p shipper-core --lib— 1848 pass.cargo test -p shipper-core --tests— 39+ pass.cargo test -p shipper-cli— full suite green.cargo test -p shipper— OK (no tests yet in façade — it's athin re-export).
cargo clippy --workspace --all-targets --all-features— clean.cargo fmt --all --check— clean.target/debug/shipper --version→shipper 0.3.0-rc.1.