Error out if naming clashes with homonymous sub-dependencies

[straight-shoota]

This has come up in #190 but it's a different issue. Consider the following shard specs:

name: parent-project
dependencies:
  foo:
    github: foo/foo
  bar:
    github: bar/bar

name: foo
dependencies:
  bar:
    github: other/bar

Installing the dependencies for parent-project there will be an issue because the dependency bar points to different locations. Now, one of these might just be a mirror, in which case it should work completely fine (assuming both are up to date).

But the dependencies might also point to a fork or two entirely different shards which just happen to share the same identifier. Note that this is not about the original name of these dependencies but the local keys used for each dependency.

An easily reproducable example is this shard file:

name: test-dependecy-name-clash
version: 0.1.0

dependencies:
  kemal:
    github: kemalcr/kemal
  radix:
    github: mperham/sidekiq.cr

You would expect shards to install mperham/sidekiq.cr in lib/radix but it doesn't, because it puts the kemal dependency luislavena/radix there.

Obviously, the example is rather silly and wouldn't work anyway, but it might just've come by mistake; or it could really be two shards sharing the same name. There is nothing to do about such a name clash in a decentralized system. But in such a case, shards should notify the user about having different remotes for the same dependency and not install any of them.

[jhass]

Maybe make more clear what the issue/proposed solution by prepending a "Error out if" to the issue title ;)

Otherwise I fully agree.

[ysbaddaden]

Sorry, your silly example doesn't work (wrong naming).

Do you have an actual failing test case that reproduces the issue you're trying to raise? That would outline the actual problem, and allow to iterate on how to fix it.

[straight-shoota]

What doesn't work for you with that example?

This is what I do to reproduce:

$ cat shard.yml
name: test-dependecy-name-clash
version: 0.1.0

dependencies:
  kemal:
    github: kemalcr/kemal
  radix:
    github: mperham/sidekiq.cr
$ shards install
Fetching https://github.com/kemalcr/kemal.git
Fetching https://github.com/luislavena/radix.git
Fetching https://github.com/jeromegn/kilt.git
Installing kemal (0.22.0)
Installing radix (0.3.8)
Installing kilt (0.4.0)
$ cat lib/radix/shard.yml
name: radix
version: 0.3.8

authors:
  - Luis Lavena <[email protected]>

license: MIT

I there was no name clash with kemal's dependency also named radix, lib/radix/shard.yml should contain the follwing:

name: sidekiq
version: 0.7.0

authors:
  - Mike Perham <[email protected]>

# ...

[ysbaddaden]

name: foo
version: 0.1.0

dependencies:
  radix:
    github: mperham/sidekiq.cr

$ shards
Updating https://github.com/mperham/sidekiq.cr.git
Error shard name (sidekiq) doesn't match dependency name (radix)

[ysbaddaden]

In your silly example Shards is fooled because the radix dependency comes up before and is valid, then totally skips the invalid definition —it only cares about additional constraints (version, branch, tag, ...).

[straight-shoota]

Argh... My point is that with this silly example, shards should show the name mismatch error instead of silently skipping the invalid definition.

To make this example less silly, I've created a dummy shard called radix:

name: test-dependecy-name-clash
version: 0.2.0

dependencies:
  kemal:
    github: kemalcr/kemal
  radix:
    github: straight-shoota/radix

Now, having only the last dependency would install straight-shoota/radix into lib. But together with the kemal dependency, shards installs luislavena/radix. This duplicate name issue should error out, not just silently install one of the alternatives.

There is a different behaviour if the order of dependecies is reversed:

name: test-dependecy-name-clash
version: 0.3.0

dependencies:
  radix:
    github: straight-shoota/radix
  kemal:
    github: kemalcr/kemal

$ shards update
Fetching https://github.com/straight-shoota/radix.git
Fetching https://github.com/kemalcr/kemal.git
Error resolving radix (*, ~> 0.3.8)

There will at least be an error because the version of radix does not fit, but that wouldn't happen if my radix shard had a matching version.

[ysbaddaden]

There are different things.

1. check that all dependencies point to the same source eventually;

2. check all dependencies for the current level, then go deeper with nested dependencies. For now, nested dependencies are resolved immediately as they are found, so definition order is significant (it shouldn't).

I said it before but the resolver is very very stupid, and we're starting to hit some blocks. Time to think properly, I guess.

[ysbaddaden]

I.e. depends on #1

[ysbaddaden]

This will probably never be fixed, because of the very nature of Shards and Git: they're decentralised.

bar/bar and other/bar can be distinct repositories, thus different shards, but they can be the same repository (clone) and thus be perfectly valid to use.

For example I may clone a shard, create a branch with a custom fix and be valid to be referenced in the dependency graph, even if another dependency refers to the original repository —I regularly do that in Ruby.

Unless there is the possibility to know for sure whether two git repositories are clones of each other (or not)?

[straight-shoota]

But shards can detect that there are two different dependencies of the same name and should error out by default. If one is a fork of the other and they both work fine together, there might be an option to allow it (giving one of them preference).
But in general, the solution should be to fix the dependencies.

[ysbaddaden]

I overlooked the radix/sidekiq example: that's wrong naming. There is a check but the current solver recursively checks dependencies, thus finds the correct radix dependency definition first (from kemal dependencies), then... completely overlooks the wrong naming. Yes, the current solver is bad.

Solver::Graph in the SAT solver branch should compare each dependency name with each version for every spec. See 1513ee5 ... or maybe not, because it won't check it for versions that it already knows about (for performance reasons, each git show is very slow).

[bcardiff]

My proposal for this would be to use the top level dependency override the repository.
So if every nested dependency agree, there is no problem.
But if they differ, the main shard.yml file could list the dependency in conflict with a specific source. It should combine the version restrictions, unless the top level uses a ref/branch).

[straight-shoota]

This is closely related to #299.
The basic issue here is about multiple dependencies having other dependencies of the same name but different locations:

name: myshard
dependencies: 
 foo:
   git: foo/foo
 bar:
   git: bar/bar

name: foo
dependencies: 
  baz:
    git: foo/baz

name: bar
dependencies:
  baz:
    git: bar/baz

Currently (master with #354) shards pick one of the baz dependencies and installs that. This is unexpected behaviour. It should fail to resolve dependencies by default. #299 would provide an option to override the dependency location on the top level.
But this should definitely be explicit. Top-level dependencies should not simply override sub-dependencies (in the above example if myshard directly depends on bar/baz instead of bar/bar).

[elia-franzella]

Any updates? I just run into this issue.

dependencies:
  tourmaline:
    github: protoncr/tourmaline
    version: 0.22.0
  amber:
    github: amberframework/amber
    version: 0.36.0

Here the shard pool is required both in tourmaline and amber, exept one is a fork and thus conflicts.

[bcardiff]

@elia-franzella you should be able to define a shard.override.yml in your project and choose there which pool fork you want to use. See #422 for details on the override file.

[elia-franzella]

@bcardiff thanks for the reply. Isn't that a source of conflicts too? Forks can diverge, and if they do - and they do - chances are that the whole project won't compile. The above example is one of them: creating a shard.override.yml to redirect pool to ysbaddaden/pool results in definition mismatches.
I know it might be to avoid the node_modules hell, but can't we provide a way to 'scope' those dependencies?

I'm thinking something like this:

# shard 1 from tom123
name: foo

dependencies:
  baz:
    github: organization1/baz

# shard 2 from frank123
name: bar

dependencies:
  baz:
    # fork or unrelated shard
    github: organization2/baz

# root
name: project

dependencies:
  foo:
    github: tom123/foo
  bar:
    github: frank123/bar

/lib dir:

Shard used by foo
/lib/organization1/baz
Shard used by bar
/lib/organization2/baz

This schema won't ever run into direct homonymy since github usernames are unique and so do the repos under the same username.

[straight-shoota]

That won't work. If you have two shards named baz, they likely both define a top-level Baz namespace. So even if you could install two dependencies of the same name, they wouldn't even compile anyways. There is no simple way to resolve this. You can't have two things of the same name in a global namespace (be it Crystal namespaces or dependency names). Sure, there might be some things you can do to mitigate some cases (for example when just the dependency names collide, but the code fits together for some reason), but that's rarely gonna be actually useful.
The solution is simply to avoid name clashes. That's best for everyone.

[elia-franzella]

Yes and no, I think. That would be a whole new issue: if two indipendent shards shared the same namespace that would be dangerous even if the shard names were different, right? This would not be even detected by the dependency resolver.
I was thinking abount separating homonymous namespaces from different shards by aliasing them differently by the compiler itself, but sometimes actually it makes sense to use the same namespace to extend it, so it is not the case. Uhm.

[bcardiff]

@elia-franzella it's a compromise. Having a global namespace is not ideal, but the alternative would have been to keep the organization in the require statement, which would require a different override mechanism on forks.

I'm not stating that the current situation is the best one, but is one that supports sources for local path and repos that might not have the concept of organization as github.

A shard's name is decoupled from the code location via the shard.yml. The shard's name is a shared namespace. As such using common names like pool, tasks, jobs might be problematic.

[straight-shoota]

The original issue was actually resolved in #419. Since then, shards errors on conflicting dependency names. An override can be used to resolve such a conflict.

The most recent comments are diverging from the original problem. I think this is essentially a wontfix, but feel free to start a new discussion about name-based shard disambiguation.