Dependency cli parsing #673

bcardiff · 2025-05-16T03:00:47Z

Related to #672, this PR adds the logic to parse dependencies from the cli. Still not used, but worth discussing syntax and implementation.

The Dependency class unfortunately is not fit to generate the shard.yaml. Resolver key and source are normalized. I created a separate type.

The added spec shows the various syntax supported.

To create a proper dependency, with the shard name, we need to actually fetch the shard to resolve that. This is done in DependencyDefinition.from_cli while the DependencyDefinition.parts_from_cli is just the parsing without side effects.

In general {resolver}:{source} syntax is supported, but there are some shorthands for convenience.

git@...
github:foo/bar
github:Foo/Bar#1.2.3 (which will mean = 1.2.3)
https://github.com/foo/bar and other known hosts like gitlab, bitbucket, and codeberg
https://github.com/foo/bar#1.2.3 and other known hosts like gitlab, bitbucket, and codeberg
~~https://github.com/Foo/Bar/commit/000000~~
~~https://github.com/Foo/Bar/tree/v1.2.3 (which is interpreted as a tag since it's prefixed with v)~~
~~https://github.com/Foo/Bar/tree/some/branch~~
relative and absolute paths

Some of these are maybe controversial, feel free to discuss.

If you want to play around you can use the following code.

require "./src/dependency_definition"

def t(s)
  d = Shards::DependencyDefinition.from_cli(s)
  pp! d

  puts "\nshard.yaml"
  puts(YAML.build do |yaml|
    yaml.mapping do
      d.to_yaml(yaml)
    end
  end)

  puts "\nshard.lock"
  puts(YAML.build do |yaml|
    yaml.mapping do
      d.dependency.to_yaml(yaml)
    end
  end)
end

t("github:crystal-lang/crystal-db")

t("github:crystal-lang/crystal-db#0.13.0")

which will output

d # => #<Shards::DependencyDefinition:0x1011fbbd0
 @dependency=
  #<Shards::Dependency:0x101217e60
   @name="db",
   @requirement=*,
   @resolver=
    #<Shards::GitResolver:0x10120f200
     @local_path=
      "/Users/bcardiff/.cache/shards/github.com/crystal-lang/crystal-db.git",
     @name="unknown",
     @origin_url="https://github.com/crystal-lang/crystal-db.git",
     @source="https://github.com/crystal-lang/crystal-db.git",
     @updated_cache=true>,
   @resolver_key=nil,
   @source=nil>,
 @resolver_key="github",
 @source="crystal-lang/crystal-db">

shard.yaml
---
db:
  github: crystal-lang/crystal-db

shard.lock
---
db:
  git: https://github.com/crystal-lang/crystal-db.git
d # => #<Shards::DependencyDefinition:0x1011fb240
 @dependency=
  #<Shards::Dependency:0x1012179b0
   @name="db",
   @requirement=Shards::VersionReq(@patterns=["~> 0.13.0"]),
   @resolver=
    #<Shards::GitResolver:0x10120f200
     @local_path=
      "/Users/bcardiff/.cache/shards/github.com/crystal-lang/crystal-db.git",
     @name="unknown",
     @origin_url="https://github.com/crystal-lang/crystal-db.git",
     @source="https://github.com/crystal-lang/crystal-db.git",
     @updated_cache=true>,
   @resolver_key=nil,
   @source=nil>,
 @resolver_key="github",
 @source="crystal-lang/crystal-db">

shard.yaml
---
db:
  github: crystal-lang/crystal-db
  version: 0.13.0

shard.lock
---
db:
  git: https://github.com/crystal-lang/crystal-db.git
  version: 0.13.0

src/dependency.cr

spec/unit/dependency_spec.cr

src/dependency.cr

ysbaddaden · 2025-05-16T08:57:09Z

I think I'd still go the other way around: from a CLI arg that directly targets a resolver and have it parse the URL into a dependency, and fallback to iterate each resolver to parse the URL for known hosts when no CLI arg is specified, and fail asking for an explicit resolver when we can't know what the remote is.

straight-shoota · 2025-05-16T09:24:15Z

I would go the other way: encode the full information in a single URI.
This should work well. Most of the time, the natural formats are already URI-compatible. That's a fully descriptive, self-contained format that's easy to reuse in other contexts.

The implementation of different strategies is quite simple: We parse the value as a URI and route it to a resolver-specific parser based on the scheme (e.g. git, github, git+https, etc. go to GitResolver).

If the URI scheme does not indicate a specific resolver (e.g. https), we fall back to heuristics based on the full URI (e.g. based on known hostname, or file extension) or potentially even trial-and-error through possible resolvers (very optional).

src/dependency.cr

bcardiff · 2025-05-16T11:24:52Z

For GitHub, from the clone menu we should support

https://github.com/owner/repo.git
git@github.com:owner/repo.git

I want to support the most common format the user can copy paste directly.

And I want to preserve as much as possible user intent. That's why I don't want the Resolver to offer a canonical representation of the dependency for the shard.yaml. Eg: urls are downcased in the normalization and that will look odd.

I saw mentions of git+ssh somewhere. IIUC those will be git@github.com:owner/repo.git, am I missing another pattern?

Thanks for the feedback.

bcardiff · 2025-05-16T11:28:07Z

Oh and should I move parsing and rendering to a DependencyDefinition type to avoid having these changes in dependency but not used in all places?

straight-shoota · 2025-05-16T11:59:24Z

We could probably turn the named tuple return type from .parts_from_cli into a dedicated type which holds this information.

bcardiff · 2025-05-16T13:56:42Z

I think I incorporated all of the feedback.

straight-shoota · 2025-05-16T14:08:01Z

spec/unit/dependency_definition_spec.cr

+      expect_parses("github:Foo/Bar@1.2.3", "github", "Foo/Bar", VersionReq.new("~> 1.2.3"))
+
+      # GitHub urls
+      expect_parses("https://github.com/foo/bar", "github", "foo/bar", Any)


thought: I'm wondering about why this gets normalized to github resolver instead of "git", "https://github.com/foo/bar".
Both options are valid. So we only need to decide which one we want to pick.
I suppose the reason for github is that it's more concise? That's nice but not strictly necessary.
git would be closer to the original input.

For me closer to the intent is to have github: foo/bar, because I'm assuming the user is copy-pasting the url in the browser. We do preserve the format with trailing .git as those are copied from the clone popup.

At some point maybe it's worth configuring shards to map all github source to be resolved in some specific way, but is a separate story.

spec/unit/dependency_definition_spec.cr

src/dependency_definition.cr

straight-shoota · 2025-05-19T13:04:35Z

spec/unit/dependency_definition_spec.cr

+        expect_parses("..\\relative\\windows", "path", "../relative/windows", Any)
+      {% end %}
+      # Path file schema
+      expect_parses("file://#{local_relative}", "path", local_relative, Any)


issue: This is technically incorrect. The URL file://a/releative/path means path /relative/path on host a. The // indicates an authority.

The correct URL for a local path would be file:a/relative/path.

I believe some applications implicitly interpret file://a/b/c as file:a/b/c because this is a common mistake.
The file scheme is not intended for remote access. We can consider doing that, but I don't think it's a good idea as it would solidify invalid behaviour.
Instead, the best way to handle this is probably to error if the host of a file URI is anything else but empty or localhost.

If possible, we can send a warning about the preferred form, but still accept it.

Let's be nice with our users. I've used that format before myself.

straight-shoota · 2025-05-20T08:04:31Z

Following up on #673 (comment), I propose an alternative implementation, fully based on URI parsing in #678

IMO this makes it much easier to handle things. The different routes are clearly decided by the URI scheme. All parsing is safe and sound using stdlib's URI class.

straight-shoota · 2025-05-20T10:24:26Z

For reference, VCS dependencies in PIP: https://pip.pypa.io/en/stable/topics/vcs-support/

bcardiff · 2025-05-20T16:58:17Z

The alternative implementation seems good. We can close this PR then in favor of the other. Anything else that is missing?, because it's marked as draft.

straight-shoota · 2025-05-20T20:34:29Z

#678 is a patch against the branch of this PR. We should merge that in and then continue here.

bcardiff · 2025-05-21T19:21:21Z

Commit pushed into this branch

straight-shoota · 2025-05-21T20:21:32Z

The windows test is failing because for relative URIs we expect .starts_with?("./"), .starts_with?("../") to recognize a relative path by prefix.
If we want to support path's with backslash separators as well, we need to add .starts_with?(".\\"), .starts_with?("..\\")

src/dependency_definition.cr

Co-authored-by: Johannes Müller <straightshoota@gmail.com>

straight-shoota · 2025-05-31T20:19:22Z

src/dependency_definition.cr

+          source = uri.to_s
+          # narrow down requirement
+          requirement = Any
+          if source.includes?("@")
+            source, version = source.split("@")
+            requirement = VersionReq.new("~> #{version}")
+          end


issue: Splitting on "@" over the entire URL is error prone. It would also match the user info portion of a URI.
git:user@host:repo parses into source user with version requirement ~> host:repo.
This mechanism must be restricted to the path of the URI.

Even then, there's a chance for misinterpretation because @ is a completely valid character in a URI path. Might not be likely to encounter shards at such a path, but I'm wondering if it's worth it. IMO the @ syntax is not essential.
We could consider a different format for expressing version constraints concisely in the URL.

Nix flake uses query parameters to encode version information (e.g. git+https://example.com/foo/bar?branch=master)
https://nix.dev/manual/nix/2.28/command-ref/new-cli/nix3-flake#examples

I think this is not entirely elegant because query parameters have a different purpose. They're supposed to be resolved by the remote party.

The URI fragment is more elegant for this. It represents resource-internal information resolved by the local party.
Our URIs point to shard repositories, that's the resource. And navigating to a specific portion of a resource is the purpose of a fragment.

So URLs would look like this: github:Foo/Bar#1.2.3, git+https://example.com/foo/bar#1.2.3.

We can further enhance this by adding support for parameters to the fragment, thus being able to resolve fragments like #branch=master, or #tag=1.2.3-rc1.

Wouldn't that # force the need of quoting in the shell?

No. You can use URLs with fragments perfectly fine in shell commands.

The shell considers # as a comment only if it's preceded by whitespace.

$ echo foo#bar foo#bar $ echo foo #bar foo

Using fragments for version numbers with the same semantics currently have @ seems good to me.

Tag and branch (and revision/commit) can come later imo.

It may work, but other package managers (cargo, bundler, ...) use @version, so using something else like #version is confusing, and # has a special meaning in shells (comment), which only increases the confusion.

The fact shards encourage semver, calver seems good enough for me to make @ or whatever syntax that mentions just a version to mean ~>.

I would leave out for now any other version operator.

I don't mean to jump. I mean to discuss and decide. I don't want to not support ~> version restriction out in this first cut.

The point is not about whether it's generally reasonable or practical to interpret a plain version as ~>.
My concern is that in all other applications that use a similar reference scheme, a plain version means exactly that version. Not a lower bound / pessimistic restriction for the actual version.
Introducing different semantics than what people are used from other tools causes confusion. I think we should try to avoid that.

Definitively @ is = in my mind. # is ok if we don't find anything better.

Ok. So for ~> it should be github:foo/bar#~>1.2.3 ?

I think is important to support ~> so we can have a good copy paste instruction with version restriction and we should encourage ~> as much as possible.

we should encourage ~> as much as possible.

This might be diverting this discussion even more than it already is. But I'm not sure I can agree on that.
What's the reason?
I don't think we have ever recommended version restrictions anywhere in installation instructions for any shard before.

bcardiff · 2025-06-08T03:56:06Z

Made some changes to use # instead of @ while supporting it also in other syntax like git@ https:// etc and not just github:

beta-ziliani · 2025-06-10T22:17:17Z

spec/unit/dependency_definition_spec.cr

+      {% end %}
+      # Path file schema
+      expect_raises Shards::Error, "Invalid file URI" do
+        Shards::DependencyDefinition.parts_from_cli("file://#{local_relative}")


I prefer a warning or a message explicitly explaining why this is not accepted

We can iterate on the warnings and messages once this is integrated in the CLI, but yes 👍

beta-ziliani · 2025-06-10T22:20:10Z

I gave my opinion on the high level of it. I think it's loable to center on the user, in aspects like being able to copy-paste from the browser. Thanks Branch!

luislavena · 2025-06-11T13:18:38Z

Folks, I think we are too much into the weeds about the pessimistic comparator versus an specific version that we are forgetting that not all shells are going to parse ~> as you expect.

Point of example, zsh:

$ cat shards
#!/usr/bin/env sh

echo "called with $@"

$ ./shards add github:foo/bar
called with add github:foo/bar

But fails with pessimistic one:

$ ./shards add github:foo/bar#~>1.2.3

$

$ cat 1.2.3
called with add github:foo/bar#~

This will force the user to quote the parameter:

$ ./shards add "github:foo/bar#~3.2.1"
called with add github:foo/bar#~3.2.1

bcardiff · 2025-06-11T14:28:44Z

Ok, leaving # as exact version. (Or I can drop that too).

Adding a version: manually is easy, definitely, but this whole thing is to avoid the user editing the shard.yml manually if possible. Leaving version information out seems a miss to me.

Deferring the discussion on how to support ~> 1.2.3 as a uri-sh parameter for later (maybe something along the way of #compat=1.2.3 to avoid the >.

I do want to move in to the actual code of changing the yaml based on this 😅

bcardiff added 2 commits May 15, 2025 23:39

Add parts_from_cli helper method to parse cli arguments

1e95757

Allow Dependency to preserve resolver key and source

aae1479

ysbaddaden reviewed May 16, 2025

View reviewed changes

src/dependency.cr Outdated Show resolved Hide resolved

ysbaddaden reviewed May 16, 2025

View reviewed changes

spec/unit/dependency_spec.cr Outdated Show resolved Hide resolved

ysbaddaden reviewed May 16, 2025

View reviewed changes

src/dependency.cr Outdated Show resolved Hide resolved

straight-shoota reviewed May 16, 2025

View reviewed changes

src/dependency.cr Outdated Show resolved Hide resolved

Extract logic from Dependency, update supported syntax

eafca30

bcardiff force-pushed the dependency-cli-parsing branch from 20d0567 to eafca30 Compare May 16, 2025 13:56

straight-shoota reviewed May 16, 2025

View reviewed changes

bcardiff and others added 3 commits May 16, 2025 17:59

single logic for known git providers

040cbdd

normalize paths from windows to posix

53353dc

URI-based dependency parsing

91c3656

straight-shoota mentioned this pull request May 20, 2025

URI-based dependency parsing #678

Merged

straight-shoota reviewed May 20, 2025

View reviewed changes

straight-shoota added the kind:feature label May 20, 2025

Fix windows CI

47ec209

straight-shoota reviewed May 22, 2025

View reviewed changes

src/dependency_definition.cr Outdated Show resolved Hide resolved

Fix windows CI (take 2)

ac81d3c

Update dependency_definition.cr

d03ae53

Co-authored-by: Johannes Müller <straightshoota@gmail.com>

straight-shoota reviewed May 31, 2025

View reviewed changes

bcardiff added 2 commits June 8, 2025 00:43

Use # instead of @

05f3d97

Add # syntax to other source schemes

05483f2

beta-ziliani reviewed Jun 10, 2025

View reviewed changes

Make # be exact version

47831a1

Uh oh!

Dependency cli parsing #673

Are you sure you want to change the base?

Dependency cli parsing #673

Uh oh!

Conversation

bcardiff commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ysbaddaden commented May 16, 2025

Uh oh!

straight-shoota commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bcardiff commented May 16, 2025

Uh oh!

bcardiff commented May 16, 2025

Uh oh!

straight-shoota commented May 16, 2025

Uh oh!

bcardiff commented May 16, 2025

Uh oh!

straight-shoota May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcardiff May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

straight-shoota May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

straight-shoota commented May 20, 2025

Uh oh!

bcardiff commented May 20, 2025

Uh oh!

straight-shoota commented May 20, 2025

Uh oh!

bcardiff commented May 21, 2025

Uh oh!

straight-shoota commented May 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcardiff commented May 16, 2025 •

edited

Loading

straight-shoota commented May 16, 2025 •

edited

Loading

straight-shoota May 16, 2025 •

edited

Loading

bcardiff May 16, 2025 •

edited

Loading

straight-shoota May 19, 2025 •

edited

Loading

straight-shoota commented May 20, 2025 •

edited

Loading

straight-shoota Jun 10, 2025 •

edited

Loading

straight-shoota Jun 11, 2025 •

edited

Loading

beta-ziliani commented Jun 10, 2025 •

edited

Loading