Handle ambiguous dependencies and update shard.lock on if source of dependency changes change

[bcardiff]

Changing the source from git to path in the shard.yml, in the precense of a shard.lock file didn't trigger an update of the lockfile. The outdated_lockfile? logic didn't consider all the information that will be used to generate that file (resolver key and source were missing).

Now lock constraints are not added if the resolver from the shard.yml and from the shard.lock differ.

It will now handle better ambiguous sources for the same name. Before this PR one was used because the cache was accessed by name only. Now it depends on name, key and (normalized) source.

The resolver cache needed to be reset in some helper methods on integration specs to avoid misleading results. Well, now that the cache key depends not only in the name that change is not really needed, but I think is better to isolate that. The cache was designed for the solver and that is not used in the integration specs.

[waj]

This would resolve to the latest version, for example in the case of a url change. Think about moving from GitLab to GitHub, or renaming the repository. Should we try to keep the locked version and force an update in case it's not available?

[bcardiff]

I see, you mean that

1. if the shard.yml has no version restriction,
2. there is a lock pointing to the non latest version
3. the shard.yml is changed to point to another fork (and still has no version restriction)

The result is that the lock is totally discarded and the dependency will bump.

I see a change of source as a change of dependency entirerly, so I wouldn't try to keep the lock.
I also have more in mind changing the source form git to path for local overrides, there the lock is also expected to be discarded.

[waj]

But that's a problem, because there is no way to avoid that behaviour. If the locked version is honoured, there is always the option to run an update to unlock it.

For example, if you're testing something and want to use a local version of the dependency momentarily, you don't want the whole subtree of dependencies to upgrade.

I think the solution would be as simple as using a new dependency as the lock node, built using the resolver coming from the spec dependency, combined with the locked version, and probably raise a warning or something to notify the change being made.

[bcardiff]

If there is a change in the shard.yml that makes the lock not a candidate, the lock for that dependency is ignored.
I aimed for the same behaviour if the source change. The version might have differed completely between forks.

But maybe we can keep the locks of the subtree.... that would be more conservative.

[bcardiff]

On other hand, the forks can use different dependencies. Keeping locked only the ones that will still be required, to allow the others to go away, will require resolving which version in the forked source will be used.

I'm more inclined to keep the current behavior in the PR. Unlocking all dependencies and treating the change of source as a complete change of the indicated dependency.

[bcardiff]

I implemented a more conservative update.

One case that I am not sure is:

    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)}

      output = run "shards install --no-color"
    end

The feature/super branch has the 0.1.0 release as ancestor, so it's rechable and the install can be conservative. The final lock in the above example still points to 0.1.0. If update is done the branch is used, but not in the install.

Is that the right thing to do?

[waj]

@bcardiff I think that's actually independent of this PR and dependency source changes. There is in fact a TODO in the git resolver for that 😉

shards/src/resolvers/git.cr

Lines 181 to 189 in 2a87638

	def matches_ref?(ref : GitRef, version : Version)
	case ref
	when GitCommitRef
	ref =~ git_ref(version)
	else
	# TODO: check branch and tags
	true
	end
	end

[bcardiff]

So the spec I mentioned can be added later when that TODO is implemented. Yay!

[bcardiff]

Rebased on master to solve conflicts