Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved error message for version restrictions #633

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spec/integration/subcommand_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe "subcommand" do
end
end

private def with_path(path)
private def with_path(path, &)
old_path = ENV["PATH"]
ENV["PATH"] = "#{File.expand_path(path)}#{Process::PATH_DELIMITER}#{ENV["PATH"]}"
yield
Expand Down
2 changes: 1 addition & 1 deletion src/cli.cr
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,6 @@ rescue ex : Shards::ParseError
ex.to_s(STDERR)
exit 1
rescue ex : Shards::Error
Shards::Log.error { ex.message }
Shards::Log.error { ex.message.to_s }
exit 1
end
25 changes: 24 additions & 1 deletion src/commands/command.cr
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require "../lock"
require "../spec"
require "../override"
require "levenshtein"

module Shards
abstract class Command
Expand Down Expand Up @@ -73,8 +74,30 @@ module Shards
Shards::Lock.write(packages, override_path, LOCK_FILENAME)
end

def handle_resolver_errors(&)
private def log_available_tags(conflicts)
conflicts.join(separator: "\n") do |k, v|
req = v.requirement
tags = req.resolver.available_tags.reverse
req = req.requirement

if req.is_a?(Version) || (req.is_a?(VersionReq) && req.patterns.size == 1 && req.patterns[0] !~ /^(<|>|=)/)
req = "v" + req.to_s
found = Levenshtein.find(req, tags, 6)
"For #{k} the closest available tag to #{req} is: #{found}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This branch is about version restrictions. We should not confuse that with tags.
It's an implementation detail that versions are represented as tags in the version control systems.

So the base set should be available_releases instead of available_tags. For a version restriction we don't care about all the tags, only the versions.

Also, I believe we need to explicitly handle the case that no versions are available.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean simply replacing tags for release in messages and function names?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main change would be to use resolver.available_releases instead of resolve.available_tags as source of available items for this branch. And then adapt the wording as well, of course.

I can make a patch against your branch to demonstrate what I mean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that won't work for what we want to do here. If you filter already the tags, then you don't get information to display the error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll think of an alternative

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use releases but also show tags

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the top level distinction must be based on the type of restriction in order to produce an appropriate error message. For example, if the restriction is branch: foo there's no point talking about releases.

case req
when Version
  # fetch available releases
  # check for close matches
  # check for similar refs that are not versions
when VersionReq
  # fetch available releases
  # check for close matches relative to the restriction
  # check for similar refs that are not versions
when GitBranchRef, HgBranchRef, FossilBranchRef
  # fetch available branches
  # check for close matches
  # check for similar refs that are not branches
when GitTagRef, HgTagRef, FossilTagRef
  # fetch available tags
  # check for close matches
  # check for similar refs that are not tags
end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm... this is harder than thought:

  • Version is used in very specific cases, and I don't think a levenshtein diff will be useful
  • VersionReq has the "just this version" case which follows the patter of Version above. This is the only useful case.
  • Any other VersionReq is hard to comply to "close match". What is close match of ">0.3.0, <0.3.0"?
  • A failing branch fails at checkin, not at resolving, so won't be part of this case.
  • Tags never fail??

So I'm tempted to leave it as is for now.

elsif tags.empty?
"#{k} doesn't have any tag"
else
"For #{k} the last available tags are #{tags.first(5).join(", ")}"
end
end
end

def handle_resolver_errors(solver, &)
yield
rescue e : Molinillo::VersionConflict(Shards::Dependency, Shards::Spec)
Log.error { e.message }
Log.error { log_available_tags(e.conflicts) }
Copy link
Member

@straight-shoota straight-shoota Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This should probably be a single error message. The conflict information is just context for the version conflict error, not itself another error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH if some workflow expects the error output as before, changing that might break it

Copy link
Member

@straight-shoota straight-shoota Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If something depends on the full exact text of the error message, it should be expected to break.
We cannot possibly guarantee stability on that.

raise Shards::Error.new("Failed to resolve dependencies")
rescue e : Molinillo::ResolverError
Log.error { e.message }
raise Shards::Error.new("Failed to resolve dependencies")
Expand Down
2 changes: 1 addition & 1 deletion src/commands/install.cr
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module Shards

solver.prepare(development: Shards.with_development?)

packages = handle_resolver_errors { solver.solve }
packages = handle_resolver_errors(solver) { solver.solve }

if Shards.frozen?
validate(packages)
Expand Down
2 changes: 1 addition & 1 deletion src/commands/lock.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ module Shards

solver.prepare(development: Shards.with_development?)

packages = handle_resolver_errors { solver.solve }
packages = handle_resolver_errors(solver) { solver.solve }
return if packages.empty?

if print
Expand Down
2 changes: 1 addition & 1 deletion src/commands/outdated.cr
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module Shards
solver = MolinilloSolver.new(spec, override, prereleases: @prereleases)
solver.prepare(development: Shards.with_development?)

packages = handle_resolver_errors { solver.solve }
packages = handle_resolver_errors(solver) { solver.solve }
packages.each { |package| analyze(package) }

if @up_to_date
Expand Down
2 changes: 1 addition & 1 deletion src/commands/update.cr
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module Shards

solver.prepare(development: Shards.with_development?)

packages = handle_resolver_errors { solver.solve }
packages = handle_resolver_errors(solver) { solver.solve }
install(packages)

if generate_lockfile?(packages)
Expand Down
2 changes: 1 addition & 1 deletion src/package.cr
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ module Shards
end
end

private def each_executable_path(name)
private def each_executable_path(name, &)
exe = Shards::Helpers.exe(name)
yield Path["bin", exe]
yield Path["bin", name] unless name == exe
Expand Down
4 changes: 4 additions & 0 deletions src/resolvers/crystal.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ module Shards
[Version.new Shards.crystal_version]
end

def available_tags : Array(String)
[Shards.crystal_version]
end

def read_spec(version : Version) : String?
nil
end
Expand Down
12 changes: 10 additions & 2 deletions src/resolvers/fossil.cr
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,18 @@ module Shards
end
end

def available_tags : Array(String)
tags = capture("fossil tag list -R #{Process.quote(local_fossil_file)}")
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved
.split('\n')

tags.reject(&.empty?)
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved
end

protected def versions_from_tags
capture("fossil tag list -R #{Process.quote(local_fossil_file)}")
tags = capture("fossil tag list -R #{Process.quote(local_fossil_file)}")
.split('\n')
.compact_map { |tag| Version.new($1) if tag =~ VERSION_TAG }

tags.compact_map { |tag| Version.new($1) if tag =~ VERSION_TAG }
end

def install_sources(version : Version, install_path : String)
Expand Down
12 changes: 10 additions & 2 deletions src/resolvers/git.cr
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,18 @@ module Shards
end
end

def available_tags : Array(String)
tags = capture("git tag --list #{GitResolver.git_column_never}")
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved
.split('\n')

tags.reject(&.empty?)
end

protected def versions_from_tags
capture("git tag --list #{GitResolver.git_column_never}")
tags = capture("git tag --list #{GitResolver.git_column_never}")
.split('\n')
.compact_map { |tag| Version.new($1) if tag =~ VERSION_TAG }

tags.compact_map { |tag| Version.new($1) if tag =~ VERSION_TAG }
end

def install_sources(version : Version, install_path : String)
Expand Down
13 changes: 11 additions & 2 deletions src/resolvers/hg.cr
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ module Shards
rescue Error
end

def available_tags : Array(String)
tags = capture("hg tags --template #{Process.quote("{tag}\n")}")
.lines
.sort!

tags.reject(&.empty?)
end

def available_releases : Array(Version)
update_local_cache
versions_from_tags
Expand Down Expand Up @@ -221,10 +229,11 @@ module Shards
end

protected def versions_from_tags
capture("hg tags --template #{Process.quote("{tag}\n")}")
tags = capture("hg tags --template #{Process.quote("{tag}\n")}")
.lines
.sort!
.compact_map { |tag| Version.new($1) if tag =~ VERSION_TAG }

tags.compact_map { |tag| Version.new($1) if tag =~ VERSION_TAG }
end

def install_sources(version : Version, install_path : String)
Expand Down
4 changes: 4 additions & 0 deletions src/resolvers/path.cr
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ module Shards
[spec(nil).version]
end

def available_tags : Array(String)
[spec(nil).version.to_s]
end

def local_path
source
end
Expand Down
2 changes: 2 additions & 0 deletions src/resolvers/resolver.cr
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ module Shards
end
end

abstract def available_tags : Array(String)

abstract def available_releases : Array(Version)

def latest_version_for_ref(ref : Ref?) : Version
Expand Down
Loading