search

crystal-lang / shards

Dependency manager for the Crystal language
Other
763 stars 100 forks source link

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

Closed bcardiff closed 4 years ago

bcardiff commented 4 years ago

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.

bcardiff commented 4 years ago

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 commented 4 years ago

@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 😉 https://github.com/crystal-lang/shards/blob/2a8763892bcc1b6869f1b9913db34660e245df77/src/resolvers/git.cr#L181-L189

bcardiff commented 4 years ago

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

bcardiff commented 4 years ago

Rebased on master to solve conflicts