Rework of dependency and requirements #354

waj · 2020-04-16T02:22:00Z

This is a refactor of dependency requirements (specifications of which versions are suitable for each dependency).

Currently, each dependency has different fields (version, branch, tag, commit) that declares the requirement. But this model is imprecise (allows invalid states), difficult to reason in code (version is sometimes a pattern, others an exact version) and requires constant parsing of the version field (for example to differentiate patterns, or exact version).

With this change, each Dependency now has only three fields: name, resolver and requirement. Also note that the relationship between Dependency and Resolver is now inverted. (In a future refactor I also plan to remove the name from Resolver making it totally independent from the dependency that it pretends to install).

Requirement is actually a union of these types (structs):

Version: represents an exact version already resolved. Used in dependencies read from lock files and internally during dependency resolution.
VersionReq: a version pattern. This corresponds to the version field entered in the shard.yml
Ref: an abstract struct representing any reference specific for the resolver
Any: a wildcard, specifying that any version is suitable

Currently we only have only one resolver (git) that defines subclasses of Ref:

GitBranchRef: corresponds to the branch attribute in shard.yml
GitTagRef: corresponds to the tag attribute in shard.yml
GitCommitRef: corresponds to the commit attribute in shard.yml
GitHeadRef: used internally when there is no version specified and no tags

The parsing of Ref is delegated to the resolver.
Also, printing of exact versions is delegated to resolvers, thus producing better output without introducing ad-hoc code.

spec/unit/spec_spec.cr

src/config.cr

src/requirement.cr

src/resolvers/git.cr

src/resolvers/path.cr

straight-shoota · 2020-04-16T10:41:03Z

The internal refactoring looks nice.

But I'm hesitant about the changes to parsing dependencies. They violate quite a few properties that
have been established in #306 and previous discussions. For example, it was agreed that extraneaous attributes in dependency mappings should be ignored to enable forward compatibility. And that YAML parsing should be isolated and not depend on resolvers. Requiring the resolver url to be the first entry in the dependency mapping is also a huge breaking change. I can't see how we could possibly accept that. The SPEC does not define a specific order of keys, so the implementation must not assume any.
Now this breaks with all of that, and I don't see any immediate benefit wich would justify deriving from the previous decisions.
Maybe you can shed some light on that? Is it necessary to break behaviour and why?

I've looked at the dependency mappings available on shardbox.org to get some stats: There are 720 dependencies (about 11%) where the resolver is not the first k/v pair. These would be broken with this change. When taking only dependencies defined by latest releases into account, the number is still 122 broken dependencies (but that's even 20%).

Additional, undefined properties on the other hand are less frequent. There are only 35 instances (about 0,5 %) and they all seem to be misspellings of version. So in this regard, a breaking change would be acceptable. But it would break forward compatibility in case we intend to introduce additional properties at some point.

The stats only take dependencies into account. I didn't look at dev_dependencies as I don't think additional data is necessary.

waj · 2020-04-16T12:57:04Z

Big oops! Thank you @straight-shoota for being right there. I thought it could be acceptable to break compatibility because fixing the yml would be an easy thing, but that would also break the parsing of previous versions which is not acceptable. I don't know what I was thinking... sorry and thanks.

I'll change the parsing to keep it compatible. Probably by parsing each dependency attributes to a hash, figure out the resolver type and pass that hash to the resolver to define the requirement.

Now, what do we win by allowing the resolver key to not be the first entry in each map? Maybe it's late to change that, but I think it improves readability of the spec.yml.

RX14 · 2020-04-16T15:56:33Z

I just want to say having shardbox to give statistics about these kinds of changes has been amazing and affected real data-driven change to shards. Thank you so much @straight-shoota!

waj · 2020-04-16T16:28:57Z

Absolutely, shardbox is awesome!

I just pushed the changes that revert the (in)compatibility of the parsing.

I also replaced the Dependency.new(pull) with Dependency.from_yaml because I didn't like that little game with tuples to workaround issues with nilables.

bcardiff · 2020-04-16T17:46:51Z

src/resolvers/git.cr

-    private def git_refs(version)
-      case version
+    def parse_requirement(params : Hash(String, String)) : Requirement
+      params.each do |key, value|


Should we raise an Ambiguous dependency requirement for ... if more than one branch, tag, commit is specified instead of honoring the first one?

Alternatively, pick the most specific one version > tag > branch.

Actually a tag could be more specific than a version with a pattern. But they also could be conflicting if a version is not reachable in a branch.

If anything, I would say we could raise instead of considering just the first one. @straight-shoota do you think this would be a breaking change?

We could implement a stricter parser if we want, for the current shard.yml, but be more relaxed when the solver is scanning released versions.

I'd say we could define this later. There is currently not defined behavior with conflicting attributes and this PR is not changing that.

Ok. I’m out of feedback :-)

JFTR: Shardbox database shows no dependencies with more more than one requirement restriction. So breaking things is not a concern.

bcardiff · 2020-04-16T18:00:26Z

I've just noticed the lock file generated after this refactor as follows. Which is semantically fine, but just wondering if the change is intended or not.

version: 1.0
shards:
  molinillo:
---    github: crystal-lang/crystal-molinillo
+++    git: https://github.com/crystal-lang/crystal-molinillo.git
    version: 0.1.0

straight-shoota · 2020-04-16T18:12:40Z

@bcardiff I guess it doesn't matter much either way. Using explicit git is a bit more verbose, but might have a slight advantage towards forward compatibility. If we add more shortcut keys, older shards version would still be able to install from the lock file.

bcardiff · 2020-04-16T18:16:49Z

Yes, I agree it can still work and it might even be a good choice. But since it wasn't stated as an intended change, I wanted to double-check.

waj · 2020-04-16T18:20:16Z

Actually that changed since #306, not my fault 😇

I think it's ok, if we think the lock file is saving a normalized definition of the dependencies

straight-shoota · 2020-04-17T05:17:53Z

I would have appreciated to get a chance at properly reviewing the updated changes :/

Rework of dependency and requirements

634c0a5

bcardiff reviewed Apr 16, 2020

View reviewed changes

spec/unit/spec_spec.cr Show resolved Hide resolved

src/config.cr Show resolved Hide resolved

src/requirement.cr Show resolved Hide resolved

src/resolvers/git.cr Show resolved Hide resolved

src/resolvers/path.cr Show resolved Hide resolved

waj added 2 commits April 16, 2020 13:21

Bring back compliance of dependency parsing with SPEC

4b421a9

Replace Dependency.new(YAML::PullParser) with Dependency.from_yaml

fe411a1

bcardiff reviewed Apr 16, 2020

View reviewed changes

bcardiff approved these changes Apr 17, 2020

View reviewed changes

waj merged commit 4395d37 into crystal-lang:master Apr 17, 2020

waj deleted the rework-dependency-requirement branch April 17, 2020 04:57

straight-shoota mentioned this pull request Apr 17, 2020

Remove obosolete Dependency#to_human_requirement #356

Merged

waj mentioned this pull request Apr 17, 2020

Format dependency and requirements #358

Merged

straight-shoota mentioned this pull request Apr 19, 2020

Error out if naming clashes with homonymous sub-dependencies #191

Closed

waj mentioned this pull request Apr 29, 2020

Fix: shard.lock is always overwritten even when dependencies are up to date #370

Merged

jhass mentioned this pull request May 25, 2020

Always add the commit hash to the lock file #242

Open

bcardiff added this to the v0.11.0 milestone May 27, 2020

taylor pushed a commit to vulk/shards that referenced this pull request Aug 11, 2020

Rework of dependency and requirements (crystal-lang#354)

c80f0c3

f-fr pushed a commit to f-fr/shards that referenced this pull request Jan 2, 2021

Rework of dependency and requirements (crystal-lang#354)

ca64dbb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework of dependency and requirements #354

Rework of dependency and requirements #354

waj commented Apr 16, 2020

straight-shoota commented Apr 16, 2020 •

edited

Loading

waj commented Apr 16, 2020

RX14 commented Apr 16, 2020 •

edited

Loading

waj commented Apr 16, 2020

bcardiff Apr 16, 2020

straight-shoota Apr 16, 2020

waj Apr 16, 2020

waj Apr 17, 2020

bcardiff Apr 17, 2020

straight-shoota Apr 17, 2020

bcardiff commented Apr 16, 2020

straight-shoota commented Apr 16, 2020

bcardiff commented Apr 16, 2020

waj commented Apr 16, 2020

straight-shoota commented Apr 17, 2020

Rework of dependency and requirements #354

Rework of dependency and requirements #354

Conversation

waj commented Apr 16, 2020

straight-shoota commented Apr 16, 2020 • edited Loading

waj commented Apr 16, 2020

RX14 commented Apr 16, 2020 • edited Loading

waj commented Apr 16, 2020

bcardiff Apr 16, 2020

Choose a reason for hiding this comment

straight-shoota Apr 16, 2020

Choose a reason for hiding this comment

waj Apr 16, 2020

Choose a reason for hiding this comment

waj Apr 17, 2020

Choose a reason for hiding this comment

bcardiff Apr 17, 2020

Choose a reason for hiding this comment

straight-shoota Apr 17, 2020

Choose a reason for hiding this comment

bcardiff commented Apr 16, 2020

straight-shoota commented Apr 16, 2020

bcardiff commented Apr 16, 2020

waj commented Apr 16, 2020

straight-shoota commented Apr 17, 2020

straight-shoota commented Apr 16, 2020 •

edited

Loading

RX14 commented Apr 16, 2020 •

edited

Loading