Handle ambiguous dependencies and update shard.lock on if source of d…

…ependency changes change (crystal-lang#419)

* Update lock file if source change (but not version)

Ensure resolver cache does not bother when parsing shard.lock and .shards.info files in integration specs

Include key, name and source in the resolver cache

* Refactor source normalization

* Ensure it fails on ambiguous sources

* Fix spec that where using git: git_path instead of git_url

* Skip adding lock if resolver of lock and dependency changes

* Mimic proper fork in specs

* Preserve locked version on changed source

Emit warning on source change
Reject lock file on install --production if source changed

* Add spec for updating to forked branch

* Code clean-up

spec/integration/install_spec.cr

@@ -198,15 +198,15 @@ describe "install" do
   it "resolves dependency spec at locked commit" do
     create_git_repository "locked"
     create_git_release "locked", "0.1.0"
-    create_git_release "locked", "0.2.0", {dependencies: {pg: {git: git_path("pg")}}}
+    create_git_release "locked", "0.2.0", {dependencies: {pg: {git: git_url(:pg)}}}
 
     metadata = {
       dependencies: {
-        "locked": {git: git_path(:"locked"), branch: "master"},
+        "locked": {git: git_url(:locked), branch: "master"},
       },
     }
     lock = {
-      "locked": "0.1.0+git.commit.#{git_commits(:"locked").last}",
+      "locked": "0.1.0+git.commit.#{git_commits(:locked).last}",
     }
     with_shard(metadata, lock) { run "shards install" }
 
@@ -265,6 +265,71 @@ describe "install" do
     end
   end
 
+  it "keeps installed version if possible when dependency source changed" do
+    metadata = {dependencies: {awesome: {git: git_url(:forked_awesome)}}}
+    lock = {awesome: "0.1.0"}
+
+    with_shard(metadata, lock) do
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:awesome)}
+
+      output = run "shards install --no-color"
+
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:forked_awesome)}
+      assert_installed "awesome", "0.1.0", source: {git: git_url(:forked_awesome)}
+
+      output.should contain("Ignoring source of \"awesome\" on shard.lock")
+    end
+  end
+
+  it "keeps nested dependencies locked when main dependency source changed" do
+    metadata = {dependencies: {awesome: {git: git_url(:forked_awesome)}}}
+    lock = {awesome: "0.1.0", d: "0.1.0"}
+
+    with_shard(metadata, lock) do
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:awesome)}
+      assert_locked "d", "0.1.0", source: {git: git_url(:d)}
+
+      output = run "shards install --no-color"
+
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:forked_awesome)}
+      assert_locked "d", "0.1.0", source: {git: git_url(:d)}
+      assert_installed "awesome", "0.1.0", source: {git: git_url(:forked_awesome)}
+      assert_installed "d", "0.1.0", source: {git: git_url(:d)}
+
+      output.should contain("Ignoring source of \"awesome\" on shard.lock")
+    end
+  end
+
+  it "fails if shard.lock and shard.yml has different sources" do
+    # The sources will not match, so the .lock is not valid regarding the specs
+    metadata = {dependencies: {awesome: {git: git_url(:forked_awesome)}}}
+    lock = {awesome: "0.1.0", d: "0.1.0"}
+
+    with_shard(metadata, lock) do
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:awesome)}
+
+      ex = expect_raises(FailedCommand) { run "shards install --production --no-color" }
+      ex.stdout.should contain("Outdated shard.lock (awesome source changed)")
+      ex.stderr.should be_empty
+    end
+  end
+
+  it "fails if shard.lock and shard.yml has different sources with incompatible versions." do
+    # User should use update command in this scenario
+    # forked_awesome does not have a 0.3.0
+    # awesome has 0.3.0
+    metadata = {dependencies: {awesome: {git: git_url(:forked_awesome)}}}
+    lock = {awesome: "0.3.0"}
+
+    with_shard(metadata, lock) do
+      assert_locked "awesome", "0.3.0", source: {git: git_url(:awesome)}
+
+      ex = expect_raises(FailedCommand) { run "shards install --production --no-color" }
+      ex.stdout.should contain("Maybe a commit, branch or file doesn't exist?")
+      ex.stderr.should be_empty
+    end
+  end
+
   it "install subdependency of new dependency respecting lock" do
     metadata = {dependencies: {c: "*", d: "*"}}
     lock = {d: "0.1.0"}
@@ -743,4 +808,20 @@ describe "install" do
       stdout.should contain(%(Using --ignore-crystal-version was not needed. All shards are already compatible with Crystal 1.1.0))
     end
   end
+
+  it "fails on conflicting sources" do
+    create_git_repository "intermediate"
+    create_git_release "intermediate", "0.1.0", {
+      dependencies: {awesome: {git: git_url(:awesome)}},
+    }
+
+    metadata = {dependencies: {
+      intermediate: "*",
+      awesome:      {git: git_url(:forked_awesome)},
+    }}
+    with_shard(metadata) do
+      ex = expect_raises(FailedCommand) { run "shards install --no-color" }
+      ex.stdout.should contain("Error shard name (awesome) has ambiguous sources")
+    end
+  end
 end

spec/integration/spec_helper.cr

@@ -14,6 +14,10 @@ Spec.before_suite do
   setup_repositories
 end
 
+Spec.before_each do
+  Shards::Resolver.clear_resolver_cache
+end
+
 private def setup_repositories
   # git dependencies for testing version resolution:
   create_git_repository "web", "1.0.0", "1.1.0", "1.1.1", "1.1.2", "1.2.0", "2.0.0", "2.1.0"
@@ -88,7 +92,7 @@ private def setup_repositories
   create_git_repository "transitive"
   create_file "transitive", "src/version.cr", %(require "version"; puts Version::STRING)
   create_git_release "transitive", "0.2.0", {
-    dependencies: {version: {git: git_path(:version)}},
+    dependencies: {version: {git: git_url(:version)}},
     scripts:      {
       postinstall: "crystal build src/version.cr",
     },
@@ -97,7 +101,7 @@ private def setup_repositories
   create_git_repository "transitive_2"
   create_git_release "transitive_2", "0.1.0", {
     dependencies: {
-      transitive: {git: git_path(:transitive)},
+      transitive: {git: git_url(:transitive)},
     },
     scripts: {
       postinstall: "../transitive/version",
@@ -113,8 +117,8 @@ private def setup_repositories
   create_git_release "binary", "0.2.0", {executables: ["foobar", "baz", "foo"]}
 
   create_git_repository "c"
-  create_git_release "c", "0.1.0", {dependencies: {d: {git: git_path("d"), version: "0.1.0"}}}
-  create_git_release "c", "0.2.0", {dependencies: {d: {git: git_path("d"), version: "0.2.0"}}}
+  create_git_release "c", "0.1.0", {dependencies: {d: {git: git_url(:d), version: "0.1.0"}}}
+  create_git_release "c", "0.2.0", {dependencies: {d: {git: git_url(:d), version: "0.2.0"}}}
   create_git_repository "d"
   create_git_release "d", "0.1.0"
   create_git_release "d", "0.2.0"
@@ -124,6 +128,27 @@ private def setup_repositories
   create_git_release "incompatible", "0.2.0", {crystal: "0.2.0"}
   create_git_release "incompatible", "0.3.0", {crystal: "0.4"}
   create_git_release "incompatible", "1.0.0", {crystal: "1.0.0"}
+
+  create_git_repository "awesome"
+  create_git_release "awesome", "0.1.0", {
+    dependencies: {d: {git: git_url(:d)}},
+  }
+  # Release v0.1.0 is the same in awesome and forked_awesome
+  create_fork_git_repository "forked_awesome", "awesome"
+  # But v0.2.0 is not, they might be different
+  create_git_release "forked_awesome", "0.2.0", {
+    name:         "awesome",
+    dependencies: {d: {git: git_url(:d)}},
+  }
+  create_git_release "awesome", "0.2.0", {
+    dependencies: {d: {git: git_url(:d)}},
+  }
+  # Release v0.3.0 is only available in the original
+  create_git_release "awesome", "0.3.0", {
+    dependencies: {d: {git: git_url(:d)}},
+  }
+  checkout_new_git_branch "forked_awesome", "feature/super"
+  create_git_commit "forked_awesome", "Starting super feature"
 end
 
 private def assert(value, message, file, line)
@@ -134,13 +159,21 @@ private def refute(value, message, file, line)
   fail(message, file, line) if value
 end
 
-def assert_installed(name, version = nil, file = __FILE__, line = __LINE__, *, git = nil)
+def assert_installed(name, version = nil, file = __FILE__, line = __LINE__, *, git = nil, source = nil)
   assert Dir.exists?(install_path(name)), "expected #{name} dependency to have been installed", file, line
+  Shards::Resolver.clear_resolver_cache # Parsing Shards::Info might use cache of resolvers. Avoid it
+  info = Shards::Info.new(install_path)
+  dependency = info.installed[name]?
 
-  if version
+  if dependency && version
     expected_version = git ? "#{version}+git.commit.#{git}" : version
-    info = Shards::Info.new(install_path)
-    info.installed[name]?.try(&.requirement).should eq(version expected_version), file, line
+    dependency.requirement.should eq(version expected_version), file, line
+  end
+
+  if dependency && source
+    expected_source = source_from_named_tuple(source)
+    actual_source = source_from_resolver(dependency.resolver)
+    assert expected_source == actual_source, "expected #{name} dependency to have been installed using #{expected_source}", file, line
   end
 end
 
@@ -160,18 +193,34 @@ def assert_installed_file(path, file = __FILE__, line = __LINE__)
   assert File.exists?(File.join(install_path(name), path)), "Expected #{path} to have been installed", file, line
 end
 
-def assert_locked(name, version = nil, file = __FILE__, line = __LINE__, *, git = nil)
+def assert_locked(name, version = nil, file = __FILE__, line = __LINE__, *, git = nil, source = nil)
   path = File.join(application_path, "shard.lock")
   assert File.exists?(path), "expected shard.lock to have been generated", file, line
+  Shards::Resolver.clear_resolver_cache # Parsing Shards::Lock might use cache of resolvers. Avoid it
   locks = Shards::Lock.from_file(path)
   assert lock = locks.shards.find { |d| d.name == name }, "expected #{name} dependency to have been locked", file, line
 
   if lock && version
     expected_version = git ? "#{version}+git.commit.#{git}" : version
-    assert expected_version == lock.requirement.as(Shards::Version).value, "expected #{name} dependency to have been locked at version #{version}", file, line
+    actual_value = lock.requirement.as(Shards::Version).value
+    assert expected_version == actual_value, "expected #{name} dependency to have been locked at version #{version} instead of #{actual_value}", file, line
+  end
+
+  if lock && source
+    expected_source = source_from_named_tuple(source)
+    actual_source = source_from_resolver(lock.resolver)
+    assert expected_source == actual_source, "expected #{name} dependency to have been locked using #{expected_source} instead of #{actual_source}", file, line
   end
 end
 
+private def source_from_named_tuple(source : NamedTuple)
+  source.to_yaml.lines.last
+end
+
+private def source_from_resolver(resolver : Shards::Resolver)
+  resolver.yaml_source_entry
+end
+
 def refute_locked(name, version = nil, file = __FILE__, line = __LINE__)
   path = File.join(application_path, "shard.lock")
   assert File.exists?(path), "expected shard.lock to have been generated", file, line

spec/integration/update_spec.cr

@@ -274,4 +274,65 @@ describe "update" do
     with_shard(metadata) { run("shards update") }
     assert_locked "local_cache", "3.0"
   end
+
+  it "updates when dependency source changed" do
+    metadata = {dependencies: {web: {path: git_path(:web)}}}
+    lock = {web: "2.1.0"}
+
+    with_shard(metadata, lock) do
+      assert_locked "web", "2.1.0", source: {git: git_url(:web)}
+
+      run "shards update"
+
+      assert_locked "web", "2.1.0", source: {path: git_path(:web)}
+      assert_installed "web", "2.1.0", source: {path: git_path(:web)}
+    end
+  end
+
+  it "keeping installed version requires constraint in shard.yml" do
+    # forked_awesome has 0.2.0
+    metadata = {dependencies: {awesome: {git: git_url(:forked_awesome), version: "~> 0.1.0"}}}
+    lock = {awesome: "0.1.0"}
+
+    with_shard(metadata, lock) do
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:awesome)}
+
+      run "shards update"
+
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:forked_awesome)}
+      assert_installed "awesome", "0.1.0", source: {git: git_url(:forked_awesome)}
+    end
+  end
+
+  it "bumps nested dependencies locked when main dependency source changed" do
+    metadata = {dependencies: {awesome: {git: git_url(:forked_awesome)}}}
+    lock = {awesome: "0.1.0", d: "0.1.0"}
+
+    with_shard(metadata, lock) do
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:awesome)}
+      assert_locked "d", "0.1.0", source: {git: git_url(:d)}
+
+      # d is not a top dependency, so it is bumped since it's required only by awesome
+      run "shards update awesome"
+
+      assert_locked "awesome", "0.2.0", source: {git: git_url(:forked_awesome)}
+      assert_locked "d", "0.2.0", source: {git: git_url(:d)}
+      assert_installed "awesome", "0.2.0", source: {git: git_url(:forked_awesome)}
+      assert_installed "d", "0.2.0", source: {git: git_url(:d)}
+    end
+  end
+
+  it "can update to forked branch after lock" do
+    metadata = {dependencies: {awesome: {git: git_url(:forked_awesome), branch: "feature/super"}}}
+    lock = {awesome: "0.1.0", d: "0.1.0"}
+
+    with_shard(metadata, lock) do
+      assert_locked "awesome", "0.1.0", source: {git: git_url(:awesome)}
+
+      run "shards update"
+
+      assert_locked "awesome", "0.2.0", git: git_commits(:forked_awesome).first
+      assert_installed "awesome", "0.2.0", git: git_commits(:forked_awesome).first
+    end
+  end
 end

spec/support/factories.cr

@@ -28,14 +28,23 @@ def create_git_repository(project, *versions)
   versions.each { |version| create_git_release project, version }
 end
 
+def create_fork_git_repository(project, upstream)
+  Dir.cd(tmp_path) do
+    run "git clone #{git_url(upstream)} #{project}"
+  end
+end
+
 def create_git_version_commit(project, version, shard : Bool | NamedTuple = true)
   Dir.cd(git_path(project)) do
     if shard
       contents = shard.is_a?(NamedTuple) ? shard : nil
       create_shard project, version, contents
     end
     Dir.cd(git_path(project)) do
-      run "git add src/#{project}.cr"
+      name = shard[:name]? if shard.is_a?(NamedTuple)
+      name ||= project
+      run "touch src/#{name}.cr"
+      run "git add src/#{name}.cr"
     end
     create_git_commit project, "release: v#{version}"
   end

src/commands/install.cr

@@ -37,7 +37,9 @@ module Shards
       private def validate(packages)
         packages.each do |package|
           if lock = locks.shards.find { |d| d.name == package.name }
-            if version = lock.requirement.as?(Shards::Version)
+            if lock.resolver != package.resolver
+              raise LockConflict.new("#{package.name} source changed")
+            elsif version = lock.requirement.as?(Shards::Version)
               validate_locked_version(package, version)
             else
               raise InvalidLock.new # unknown lock resolver
@@ -88,8 +90,8 @@ module Shards
       private def outdated_lockfile?(packages)
         return true if locks.version != Shards::Lock::CURRENT_VERSION
         return true if packages.size != locks.shards.size
-        a = packages.to_h { |x| {x.name, x.version} }
-        b = locks.shards.to_h { |x| {x.name, x.requirement.as?(Shards::Version)} }
+        a = packages.to_h { |x| {x.name, {x.resolver.class.key, x.resolver.source, x.version}} }
+        b = locks.shards.to_h { |x| {x.name, {x.resolver.class.key, x.resolver.source, x.requirement.as?(Shards::Version)}} }
         a != b
       end
     end