Automatic unlock on install and update

[waj]

This PR changes the way the content of shards.lock is used during dependency resolution. Instead of passing all o them blindly to the solver, they are traversed as a tree, starting from the dependencies specified explicitly in shards.yml, only if the version still matches, and following each sub-dependency recursively.

This fixes some issues:

• If the version in shards.yml is changed, running shards just does the update of that shard.
• Running shards update foo, if foo has dependency bar, now it's not needed to run shards update foo bar to unlock bar (unless bar is also in shards.yml)

The behavior of shards is now more similar to bundler, and I think it's a good thing. Much more intuitive and simple to use.

[ysbaddaden]

Indeed. The update command should never really lock a dependency, but aim to keep all dependencies to be as close as possible of locked versions, yet never enforce them —I liked how the 'distance' idea of the SAT solver made it explicit.

However, I'm not fond of Bundler's behavior. I believe install should always enforce locked versions. It accepts to add or remove a dependency as a convenience (unless production), but only if it doesn't affect the current dependencies. I think it's better to be upfront instead if silently updating the lock file, which can lead to some breakage because of unreviewed changes.

[ysbaddaden]

BTW: how does it behave with this change? Is it still as conservative as possible (as with the SAT solver) or is it kinda loose, possibly updating unrelated dependencies?

[waj]

The update command should never really lock a dependency, but aim to keep all dependencies to be as close as possible of locked versions, yet never enforce them

I disagree. If I execute shards update foo, I don't want to upgrade dependency bar too. No matter how small the change is. Just relaying on an arbitrary "distance" measure sounds too weak to me.

I'm not fond of Bundler's behavior

But I do. To be honest, from all the dependency managers I use from any language, it's the one that always do exactly what I'm looking for. If I change explicitly the version in the shard.yml why bother me with an update command?

I think it's better to be upfront instead if silently updating the lock file, which can lead to some breakage because of unreviewed changes.

The command clearly says which dependencies are upgraded. I can always check the changes after the upgrade and revert changes if I don't like them.

I'm sorry @ysbaddaden. Please, don't take this the wrong way. I loved the idea of the SAT solver, but in practice it didn't work. And I'm not planning to spend too much time innovating on this area. So my decision is to copy what actually works in real world. And bundler does. And we need something that works for when 1.0 arrives. After that we can continue doing experiments with other approaches. Dependency solvers algorithms are not deterministic. So, if there is a good reason, we could even have more that one algorithm embedded in Shards and let the user choose which one to use.

[RX14]

I believe install should always enforce locked versions.

This is how I prefer it too. shards install should never update - if the shard.yml and shard.lock files are out of sync, then you've forgotten to run shards update and shards should tell you this, instead of attempt to fix the behaviour, and possibly introduce a regression.

At the very least it should never modify shard.lock in --production mode, it must be strict so that you know exactly what you're running in production.

[waj]

There must be a reason why bundler (Ruby), pub (Dart), mix (Elixir), yarn (JS) all behave the same way. The rationale is:

• install: add, remove or update gems as conservative as possible. You run this command after any change on the specification file.
• update: try to get latest versions still matching the specification. This command can be run without modifying the specification file.

The Conservative Updating section of the Bundler documentation explain this.

[straight-shoota]

bundle install has --frozen option which avoids installing anything that's not in the lock file. We should probably add that too to get the behaviour that @RX14 and @ysbaddaden describe.

[RX14]

@waj that behaviour is fine in a development environment, since you're going to notice the lockfile diff when committing (you do all read your diffs before committing right?), but this is not the case when deploying to a production server.

I guess it's a nitpick, but I'd like --production to have this behaviour, or add a --frozen and --production implies --frozen.

[waj]

Of course, --production is already there and its behaviour is like --frozen and also don't install development dependencies. We could add --frozen too, but that's another story.

[RX14]

Of course, --production is already there and its behaviour is like --frozen

Oh this is already the case? I didn't realise.

[RX14]

Isn't the logic wrong here?

shards/src/commands/install.cr

Line 49 in 134850e

private def validate_locked_version(package, version)

It seems it fails if the installed version doesn't exactly match the lockfile and the installed version doesn't match the shard.yml version. Am I wrong or should it be or there instead?

[RX14]

This new behaviour looks good!

[RX14]

The if Shards.production? checks can be removed down below now right?

[ysbaddaden]

@waj We agree. I said that update should never really enforce locked dependencies but aim to be as close as possible, which means that bar shouldn't be updated unless the foo -> * -> bar requirements need a new version of bar instead of introducing a conflict (i.e. the very purpose of this patch).

I also really like Bundler, and modeled Shards after it. I'm just not fond of its update everything by default, and wish I didn't have to specify and understand a bunch of options (--conservative, --strict).

[bcardiff]

I found it odd that nested dependencies that are not mentioned in shards.yml would need to be mentioned in the shards update command. I don't think is feasible to force the user list all nested dependencies explicitly to update the top level.

If I need a nested dependency to remain at a specific version for some reason, then it will become an explicit dependency in the shard.yml.

Adding a command that will not bump nested dependencies can be added later if wanted. It is another feature.

[waj]

That behaviour (update only if needed) doesn't exist in molinillo, and I'm not sure if any other dependency manager works that way. If there is a lock, it will be respected, and raise a conflict if there is no solution. We can only decide what to unlock or not.

In bundler, when you run update --conservative foo it's similar to install when the version of foo is changed in the Gemfile: shared dependencies with other gems are not unlocked.

Right now, with this PR, both install and update are always conservative (in Bundler terms). That means, install will only unlock dependencies that have changed in shards.yml to a version that cannot be satisfied with the lock, and update will lock only dependencies (and subdependencies) that were not explicitly specified in the command line.

[waj]

@RX14 I think your comments mostly apply to the "production" mode and I didn't touch anything of that in this PR. I plan to do some refactors for production mode in a separate PR.

[RX14]

If you'd like to work on that in a separate PR that's fine but it seems the bugfix (if I'm not mistaken about it being a bug) should make it into the release.

[ysbaddaden]

That behaviour (update only if needed) doesn't exist in molinillo. We can only decide what to unlock or not.

Well, eventually it achieves kinda the same: solve with locked versions, unlock conflicting nested dependency, and try again (if I understood correctly). Maybe it's even better since it may reach directly for the latest patch, which ain't a bad idea.

[waj]

If you'd like to work on that in a separate PR that's fine but it seems the bugfix (if I'm not mistaken about it being a bug) should make it into the release.

Yes, you're right. There seems to be a bug with some uncovered cases in production mode. But it's unrelated to this PR.