diff --git a/lib/dev/deps/brew_integration.rb b/lib/dev/deps/brew_integration.rb index 5447d48..094b68a 100644 --- a/lib/dev/deps/brew_integration.rb +++ b/lib/dev/deps/brew_integration.rb @@ -44,17 +44,24 @@ def initialize(repository:, cache:, taps: [], project_dir: nil) # Install all brew dependencies. Registers taps on first call. # + # Tap registration stays outside the per-dep isolation: every install + # is predetermined to fail for the same root cause, so it surfaces as + # one integration-level failure instead of N per-dep echoes. + # # @param dependencies [Array] brew deps to install + # @raise [TapRegistrationError] if a tap cannot be registered + # @raise [PartialInstallError] if any dep fails; the rest were attempted sig { params(dependencies: T::Array[Dependency]).void } def install_all(dependencies) ensure_taps_registered - dependencies.each do |dep| + failures = collect_failures(dependencies) do |dep| if dep.metadata["cask"] install_cask(dep) else install_formula(dep) end end + raise PartialInstallError, failures if failures.any? end private diff --git a/lib/dev/deps/cmake_integration.rb b/lib/dev/deps/cmake_integration.rb index ed6d96d..39819b6 100644 --- a/lib/dev/deps/cmake_integration.rb +++ b/lib/dev/deps/cmake_integration.rb @@ -58,13 +58,21 @@ def initialize(repository:, cache:, project_root:) # Install all cmake dependencies: fetch sources, run post_install hooks, # generate deps.cmake and deps.targets.cmake. # + # On any per-dep failure the batch artifacts are NOT rewritten: a file + # generated from a partial set would silently drop the failed deps' + # variables. The previous files stay in place — stale but consistent, + # the same philosophy as the install stamp. + # # @param dependencies [Array] cmake deps to install + # @raise [PartialInstallError] if any dep fails; the rest were attempted sig { params(dependencies: T::Array[Dependency]).void } def install_all(dependencies) - dependencies.each do |dep| + failures = collect_failures(dependencies) do |dep| fetch_dep(dep) run_post_install(dep) end + raise PartialInstallError, failures if failures.any? + write_deps_cmake(dependencies) write_targets_cmake(dependencies) end diff --git a/lib/dev/deps/gh_integration.rb b/lib/dev/deps/gh_integration.rb index a4274fe..2829fd5 100644 --- a/lib/dev/deps/gh_integration.rb +++ b/lib/dev/deps/gh_integration.rb @@ -70,9 +70,11 @@ def initialize(repository:, cache:, project_root: nil) # Install all gh dependencies. # # @param dependencies [Array] gh deps to install + # @raise [PartialInstallError] if any dep fails; the rest were attempted sig { params(dependencies: T::Array[Dependency]).void } def install_all(dependencies) - dependencies.each { |dep| install(dep) } + failures = collect_failures(dependencies) { |dep| install(dep) } + raise PartialInstallError, failures if failures.any? end private diff --git a/lib/dev/deps/installer.rb b/lib/dev/deps/installer.rb index 23cfee0..c889d68 100644 --- a/lib/dev/deps/installer.rb +++ b/lib/dev/deps/installer.rb @@ -2,6 +2,7 @@ # frozen_string_literal: true require "sorbet-runtime" +require_relative "integration" module Dev module Deps @@ -12,6 +13,28 @@ module Deps class Installer extend T::Sig + # Aggregate of every failure across the whole install run, raised after + # all integrations were attempted. Entries are ordered by wave (build + # first), so root causes appear before derivative failures. Raising + # keeps the caller contract unchanged: a failed run exits non-zero and + # never writes the installed stamp. + class InstallFailedError < StandardError + extend T::Sig + + # @return [Array] one human-readable line per failure + sig { returns(T::Array[String]) } + attr_reader :entries + + # @param entries [Array] + sig { params(entries: T::Array[String]).void } + def initialize(entries) + @entries = entries + super("#{entries.size} dependency install failure(s) — " \ + "installs are idempotent, fix the causes and rerun:\n" \ + "#{entries.map { |entry| " #{entry}" }.join("\n")}") + end + end + # @param lockfile [Lockfile] lockfile reader # @param integrations [Hash{Symbol => Integration}] integration type → integration sig { params(lockfile: Lockfile, integrations: T::Hash[Symbol, T.untyped]).void } @@ -41,6 +64,8 @@ def initialize(lockfile:, integrations:) # @param env [String, nil] environment name for filtering (nil = no filtering) # @param host [String, nil] host OS name for filtering (nil = no filtering) # @return [void] + # @raise [InstallFailedError] if any integration reported failures; every + # integration was still attempted (failure isolation) sig { params(env: T.nilable(String), host: T.nilable(String)).void } def install(env: nil, host: nil) all_deps = @lockfile.read @@ -49,8 +74,8 @@ def install(env: nil, host: nil) build_deps, other_deps = all_deps.partition { |d| d.group == :build } - dispatch(build_deps) - dispatch(other_deps) + failures = dispatch(build_deps) + dispatch(other_deps) + raise InstallFailedError, failures if failures.any? end private @@ -62,12 +87,29 @@ def install(env: nil, host: nil) # integration generating batch artifacts (deps.cmake) would overwrite # its own output with each partial group. # + # A failing integration never blocks the others: the manifest declares + # no cross-integration edges, so the correct failure policy for the + # (degenerate) dependency DAG is attempt-all. Failures are collected — + # per-dep entries when the integration isolated them + # (PartialInstallError), one entry when it failed whole (e.g. a tap + # registration preamble) — and reported by the caller's aggregate. + # # @param deps [Array] dependencies to install - # @return [void] - sig { params(deps: T::Array[Dependency]).void } + # @return [Array] one entry per failure, in dispatch order + sig { params(deps: T::Array[Dependency]).returns(T::Array[String]) } def dispatch(deps) - deps.group_by { |dep| @integrations[dep.integration] }.each do |integration, typed_deps| - integration&.install_all(typed_deps) + deps.group_by { |dep| @integrations[dep.integration] }.flat_map do |integration, typed_deps| + next [] unless integration + + label = typed_deps.map(&:integration).uniq.join("+") + begin + integration.install_all(typed_deps) + [] + rescue Integration::PartialInstallError => e + e.failures.map { |name, error| "#{label}: #{name} — #{error.message}" } + rescue StandardError => e + ["#{label}: #{e.message}"] + end end end diff --git a/lib/dev/deps/integration.rb b/lib/dev/deps/integration.rb index b9385bb..0aa39f8 100644 --- a/lib/dev/deps/integration.rb +++ b/lib/dev/deps/integration.rb @@ -18,6 +18,27 @@ module Deps class Integration extend T::Sig + # Aggregate of per-dep failures within one install_all call: the loop + # attempts every dep (one bad formula must not block an unrelated + # engine download), collects what failed, and raises this at the end + # so the run still fails loudly. The Installer flattens the failures + # into its cross-integration report. + class PartialInstallError < StandardError + extend T::Sig + + # @return [Array<[String, StandardError]>] failed dep name → its error + sig { returns(T::Array[[String, StandardError]]) } + attr_reader :failures + + # @param failures [Array<[String, StandardError]>] + sig { params(failures: T::Array[[String, StandardError]]).void } + def initialize(failures) + @failures = failures + lines = failures.map { |name, error| " #{name}: #{error.message}" } + super("#{failures.size} dep install(s) failed:\n#{lines.join("\n")}") + end + end + # @param repository [Repository, nil] source adapter for this integration type # @param cache [Cache, nil] shared download cache sig { params(repository: T.nilable(Repository), cache: T.nilable(Cache)).void } @@ -36,6 +57,30 @@ def install_all(dependencies) private + # Attempt the block for every dep, isolating failures so one bad dep + # never blocks the rest of the batch. Callers raise PartialInstallError + # themselves (after skipping any batch post-processing that requires a + # fully-successful set). + # + # @param dependencies [Array] + # @yieldparam dep [Dependency] the dep to install + # @return [Array<[String, StandardError]>] failed dep name → its error + sig do + params( + dependencies: T::Array[Dependency], + blk: T.proc.params(dep: Dependency).void, + ).returns(T::Array[[String, StandardError]]) + end + def collect_failures(dependencies, &blk) + failures = T.let([], T::Array[[String, StandardError]]) + dependencies.each do |dep| + blk.call(dep) + rescue StandardError => e + failures << [dep.name, e] + end + failures + end + sig { returns(T.nilable(Repository)) } attr_reader :repository diff --git a/test/dev/deps/brew_integration_test.rb b/test/dev/deps/brew_integration_test.rb index a08de1b..5c08c8c 100644 --- a/test/dev/deps/brew_integration_test.rb +++ b/test/dev/deps/brew_integration_test.rb @@ -100,10 +100,48 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test .returns(["", "Error: No available formula", failed_status]) When "installing all" - integration.install_all(deps) + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do + integration.install_all(deps) + end - Then - raises Dev::Deps::BrewIntegration::InstallError + Then "the per-dep failure is the brew install error" + error.failures[0][1].is_a?(Dev::Deps::BrewIntegration::InstallError) + + Cleanup + FileUtils.rm_rf(dir) + end + + test "install_all attempts the remaining formulae when one fails, then raises the aggregate" do + Given "two brew dependencies, the first of which fails to install" + dir = Dir.mktmpdir("dev-brew-int-test-") + cache = Dev::Deps::Cache.new(cache_dir: dir) + integration = Dev::Deps::BrewIntegration.new(repository: Dev::Deps::BrewRepository.new, cache: cache) + deps = [ + Dev::Deps::Dependency.new(name: "bad_formula", integration: :brew, group: :build, + version: "1.0.0", hash: nil, metadata: {}), + Dev::Deps::Dependency.new(name: "good_formula", integration: :brew, group: :build, + version: "2.0.0", hash: nil, metadata: {}), + ] + integration.stubs(:brew_installed?).returns(false) + Open3.stubs(:capture3) + .with("brew", "install", "bad_formula") + .returns(["", "Error: No available formula", stub(success?: false)]) + Open3.expects(:capture3) + .with("brew", "install", "good_formula") + .returns(["", "", stub(success?: true)]) + + When "installing all and capturing the aggregate error" + error = nil + begin + integration.install_all(deps) + rescue StandardError => e + error = e + end + + Then "good_formula was still installed (Mocha-verified) and the aggregate lists bad_formula" + error.is_a?(Dev::Deps::Integration::PartialInstallError) + error.failures.map(&:first) == ["bad_formula"] + error.failures[0][1].is_a?(Dev::Deps::BrewIntegration::InstallError) Cleanup FileUtils.rm_rf(dir) diff --git a/test/dev/deps/cmake_integration_test.rb b/test/dev/deps/cmake_integration_test.rb index 68eaff9..31e9368 100644 --- a/test/dev/deps/cmake_integration_test.rb +++ b/test/dev/deps/cmake_integration_test.rb @@ -135,10 +135,12 @@ def prepopulate_dep(root, name) integration.stubs(:system).returns(false) When "installing all" - integration.install_all(deps) + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do + integration.install_all(deps) + end - Then - raises Dev::Deps::CmakeIntegration::GitCloneError + Then "the per-dep failure is the clone error" + error.failures[0][1].is_a?(Dev::Deps::CmakeIntegration::GitCloneError) Cleanup FileUtils.rm_rf(dir) @@ -165,10 +167,12 @@ def prepopulate_dep(root, name) .returns(false) When "installing all" - integration.install_all(deps) + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do + integration.install_all(deps) + end - Then - raises Dev::Deps::CmakeIntegration::GitCheckoutError + Then "the per-dep failure is the checkout error" + error.failures[0][1].is_a?(Dev::Deps::CmakeIntegration::GitCheckoutError) Cleanup FileUtils.rm_rf(dir) @@ -192,10 +196,12 @@ def prepopulate_dep(root, name) .returns(false) When "installing all" - integration.install_all(deps) + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do + integration.install_all(deps) + end - Then - raises Dev::Deps::CmakeIntegration::DownloadError + Then "the per-dep failure is the download error" + error.failures[0][1].is_a?(Dev::Deps::CmakeIntegration::DownloadError) Cleanup FileUtils.rm_rf(dir) @@ -219,10 +225,12 @@ def prepopulate_dep(root, name) integration.stubs(:system).with { |cmd, *_| cmd == "tar" }.returns(false) When "installing all" - integration.install_all(deps) + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do + integration.install_all(deps) + end - Then - raises Dev::Deps::CmakeIntegration::ExtractError + Then "the per-dep failure is the extract error" + error.failures[0][1].is_a?(Dev::Deps::CmakeIntegration::ExtractError) Cleanup FileUtils.rm_rf(dir) @@ -415,4 +423,55 @@ def prepopulate_dep(root, name) { "url" => "https://example.com/b.tar.gz" } | false | false {} | true | false end + + test "install_all leaves batch artifacts untouched when any dep fails, still attempting the rest" do + Given "two git deps (first fails to clone) and pre-existing batch artifacts" + dir = Dir.mktmpdir("dev-cmake-int-test-") + cache = Dev::Deps::Cache.new(cache_dir: File.join(dir, "cache")) + integration = Dev::Deps::CmakeIntegration.new( + repository: Dev::Deps::GitRepository.new, cache: cache, project_root: dir, + ) + previous_deps_cmake = "# previous consistent deps.cmake\n" + previous_targets_cmake = "# previous consistent deps.targets.cmake\n" + File.write(File.join(dir, "deps.cmake"), previous_deps_cmake) + File.write(File.join(dir, "deps.targets.cmake"), previous_targets_cmake) + deps = [ + Dev::Deps::Dependency.new( + name: "bad_repo", integration: :cmake, group: :app, + version: "abc123", hash: nil, + metadata: { "repo" => "https://example.com/bad_repo" }, + ), + Dev::Deps::Dependency.new( + name: "good_repo", integration: :cmake, group: :test, + version: "def456", hash: nil, + metadata: { "repo" => "https://example.com/good_repo" }, + ), + ] + integration.stubs(:system) + .with("git", "clone", "--no-checkout", "-q", "https://example.com/bad_repo", anything) + .returns(false) + integration.expects(:system) + .with("git", "clone", "--no-checkout", "-q", "https://example.com/good_repo", anything) + .returns(true) + integration.stubs(:system) + .with("git", "-c", "advice.detachedHead=false", "checkout", anything, chdir: anything) + .returns(true) + + When "installing all and capturing the aggregate error" + error = nil + begin + integration.install_all(deps) + rescue StandardError => e + error = e + end + + Then "good_repo was still cloned (Mocha-verified), artifacts are stale-but-consistent" + error.is_a?(Dev::Deps::Integration::PartialInstallError) + error.failures.map(&:first) == ["bad_repo"] + File.read(File.join(dir, "deps.cmake")) == previous_deps_cmake + File.read(File.join(dir, "deps.targets.cmake")) == previous_targets_cmake + + Cleanup + FileUtils.rm_rf(dir) + end end diff --git a/test/dev/deps/gh_integration_test.rb b/test/dev/deps/gh_integration_test.rb index c2bc74e..002c305 100644 --- a/test/dev/deps/gh_integration_test.rb +++ b/test/dev/deps/gh_integration_test.rb @@ -49,6 +49,26 @@ def download_source(_dep, archive_path) end end unless defined?(FixtureSourceGhIntegration) +# GhIntegration with per-dep install replaced by a recorder that raises for +# configured dep names — exercises install_all's failure isolation without +# any download machinery. +class ExplodingGhIntegration < Dev::Deps::GhIntegration + attr_reader :attempted + + def initialize(fail_names:, **kwargs) + super(**kwargs) + @fail_names = fail_names + @attempted = [] + end + + private + + def install(dep) + @attempted << dep.name + raise "release asset missing for #{dep.name}" if @fail_names.include?(dep.name) + end +end unless defined?(ExplodingGhIntegration) + transform!(RSpock::AST::Transformation) class Dev::Deps::GhIntegrationTest < Minitest::Test # Build a real split zstd tarball: tar + zstd the content dir, then split @@ -211,11 +231,12 @@ def build_integration(fixture_files, cache_dir) integration = build_integration(parts, File.join(dir, "cache")) When "installing tampered assets" - error = assert_raises(Dev::Deps::GhIntegration::IntegrityError) do + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do integration.install_all([dep]) end Then "no version dir is published and the staging dir is cleaned up" + error.failures[0][1].is_a?(Dev::Deps::GhIntegration::IntegrityError) error.message.include?(parts.first.basename.to_s) !File.exist?(File.join(install_dir, "5.6.1-css-83")) Dir.glob(File.join(install_dir, ".staging-*")).empty? @@ -234,10 +255,12 @@ def build_integration(fixture_files, cache_dir) integration = build_integration([zip_path], File.join(dir, "cache")) When "installing the unsupported archive" - integration.install_all([dep]) + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do + integration.install_all([dep]) + end - Then - raises Dev::Deps::GhIntegration::UnsupportedArchiveError + Then "the per-dep failure is the archive rejection" + error.failures[0][1].is_a?(Dev::Deps::GhIntegration::UnsupportedArchiveError) Cleanup FileUtils.rm_rf(dir) @@ -252,10 +275,12 @@ def build_integration(fixture_files, cache_dir) integration = build_integration(parts, File.join(dir, "cache")) When "installing with a glob that matches nothing" - integration.install_all([dep]) + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do + integration.install_all([dep]) + end Then "the mismatch is loud — never a silently empty install" - raises Dev::Deps::GhIntegration::NoMatchingAssetsError + error.failures[0][1].is_a?(Dev::Deps::GhIntegration::NoMatchingAssetsError) Cleanup FileUtils.rm_rf(dir) @@ -385,11 +410,12 @@ def build_source_integration(tarball, project_root, cache_dir) integration = build_source_integration(tarball, File.join(dir, "project"), File.join(dir, "cache")) When "installing" - error = assert_raises(Dev::Deps::GhIntegration::BuildError) do + error = assert_raises(Dev::Deps::Integration::PartialInstallError) do integration.install_all([dep]) end Then "no version dir is published and staging is cleaned up" + error.failures[0][1].is_a?(Dev::Deps::GhIntegration::BuildError) error.message.include?("UnrealEngine") !File.exist?(File.join(install_dir, "5.6.1-release")) Dir.glob(File.join(install_dir, ".staging-*")).empty? @@ -397,4 +423,36 @@ def build_source_integration(tarball, project_root, cache_dir) Cleanup FileUtils.rm_rf(dir) end + + test "install_all attempts the remaining deps when one fails, then raises the aggregate" do + Given "two gh dependencies, the first of which fails to install" + dir = Dir.mktmpdir("dev-gh-int-test-") + integration = ExplodingGhIntegration.new( + fail_names: ["UnrealEngine"], + repository: Dev::Deps::GhRepository.new, + cache: Dev::Deps::Cache.new(cache_dir: dir), + ) + deps = [ + Dev::Deps::Dependency.new(name: "UnrealEngine", integration: :gh, group: :build, + version: "5.6.1-css-83", hash: nil, metadata: {}), + Dev::Deps::Dependency.new(name: "OtherTool", integration: :gh, group: :build, + version: "1.0.0", hash: nil, metadata: {}), + ] + + When "installing all and capturing the aggregate error" + error = nil + begin + integration.install_all(deps) + rescue StandardError => e + error = e + end + + Then "the second dep was still attempted and the aggregate lists only the first" + integration.attempted == ["UnrealEngine", "OtherTool"] + error.is_a?(Dev::Deps::Integration::PartialInstallError) + error.failures.map(&:first) == ["UnrealEngine"] + + Cleanup + FileUtils.rm_rf(dir) + end end diff --git a/test/dev/deps/installer_test.rb b/test/dev/deps/installer_test.rb index e21b9da..bce7912 100644 --- a/test/dev/deps/installer_test.rb +++ b/test/dev/deps/installer_test.rb @@ -213,6 +213,117 @@ class Dev::Deps::InstallerTest < Minitest::Test FileUtils.rm_rf(dir) end + test "install still runs the remaining integrations when one raises" do + Given "a failing brew integration and a healthy cmake integration" + dir = Dir.mktmpdir("installer-test-") + lockfile = Dev::Deps::Lockfile.new(dir: Pathname(dir)) + deps = [ + Dev::Deps::Dependency.new(name: "wwise-cli", integration: :brew, group: :build, + version: "1.0", hash: "SHA256=aaa", metadata: {}), + Dev::Deps::Dependency.new(name: "googletest", integration: :cmake, group: :test, + version: "sha1", hash: nil, metadata: {}), + ] + lockfile.lock(deps) + brew_int = RecordingIntegration.new + brew_int.define_singleton_method(:install_all) do |_dependencies| + raise "Homebrew prefix is not writable" + end + cmake_int = RecordingIntegration.new + installer = Dev::Deps::Installer.new( + lockfile:, integrations: { brew: brew_int, cmake: cmake_int }, + ) + + When "running install and capturing the aggregate error" + error = nil + begin + installer.install + rescue StandardError => e + error = e + end + + Then "cmake still installed its dep and the aggregate error names the brew failure" + cmake_int.installed_deps.map(&:name) == ["googletest"] + error.is_a?(Dev::Deps::Installer::InstallFailedError) + error.message.include?("brew: Homebrew prefix is not writable") + + Cleanup + FileUtils.rm_rf(dir) + end + + test "install flattens an integration's per-dep failures into individual entries" do + Given "an integration raising PartialInstallError for one of its two deps" + dir = Dir.mktmpdir("installer-test-") + lockfile = Dev::Deps::Lockfile.new(dir: Pathname(dir)) + deps = [ + Dev::Deps::Dependency.new(name: "wwise-cli", integration: :brew, group: :build, + version: "1.0", hash: "SHA256=aaa", metadata: {}), + Dev::Deps::Dependency.new(name: "ccache", integration: :brew, group: :build, + version: "4.10", hash: "SHA256=bbb", metadata: {}), + ] + lockfile.lock(deps) + brew_int = RecordingIntegration.new + brew_int.define_singleton_method(:install_all) do |_dependencies| + raise Dev::Deps::Integration::PartialInstallError.new( + [["wwise-cli", RuntimeError.new("no bottle available")]], + ) + end + installer = Dev::Deps::Installer.new(lockfile:, integrations: { brew: brew_int }) + + When "running install and capturing the aggregate error" + error = nil + begin + installer.install + rescue StandardError => e + error = e + end + + Then "the entry names the failing dep, not just the integration" + error.is_a?(Dev::Deps::Installer::InstallFailedError) + error.entries == ["brew: wwise-cli — no bottle available"] + + Cleanup + FileUtils.rm_rf(dir) + end + + test "install orders aggregate entries build wave first" do + Given "a failing build-wave integration and a failing app-wave integration" + dir = Dir.mktmpdir("installer-test-") + lockfile = Dev::Deps::Lockfile.new(dir: Pathname(dir)) + deps = [ + Dev::Deps::Dependency.new(name: "boost", integration: :cmake, group: :app, + version: "1.90.0", hash: "SHA256=aaa", metadata: {}), + Dev::Deps::Dependency.new(name: "ccache", integration: :brew, group: :build, + version: "4.10", hash: "SHA256=bbb", metadata: {}), + ] + lockfile.lock(deps) + brew_int = RecordingIntegration.new + brew_int.define_singleton_method(:install_all) do |_dependencies| + raise "build wave failure" + end + cmake_int = RecordingIntegration.new + cmake_int.define_singleton_method(:install_all) do |_dependencies| + raise "app wave failure" + end + installer = Dev::Deps::Installer.new( + lockfile:, integrations: { brew: brew_int, cmake: cmake_int }, + ) + + When "running install and capturing the aggregate error" + error = nil + begin + installer.install + rescue StandardError => e + error = e + end + + Then "root causes (build wave) come before derivative failures (later waves)" + error.is_a?(Dev::Deps::Installer::InstallFailedError) + error.entries == ["brew: build wave failure", "cmake: app wave failure"] + + Cleanup + FileUtils.rm_rf(dir) + end + test "install skips integration types with no registered integration" do Given "a lockfile with an unregistered integration type" dir = Dir.mktmpdir("installer-test-")