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

Shards overrides #422

Merged
merged 17 commits into from
Jul 31, 2020
Merged

Shards overrides #422

merged 17 commits into from
Jul 31, 2020

Conversation

bcardiff
Copy link
Member

@bcardiff bcardiff commented Jul 22, 2020

Implements most of #412 .

Reads a shard.override.yml (or the file specified by SHARDS_OVERRIDE) to override the source and restriction of a dependency. There are no changes regarding CLI options of shards.

This allows the cases described in the motivation of #412

  • Use local working copies
  • Use a shard version despite published constraints
  • Fixing a dependency
  • Checking app against development version of a dependency

Usage sample

On a project that uses github:crystal-lang/crystal-sqlite3 it will current use crysta-db 0.8.0.

# file: shard.yml
name: sample
version: 0.1.0

dependencies:
  sqlite3:
    github: crystal-lang/crystal-sqlite3
    version: ~> 0.15.0

crystal: 0.35.1
# file: shard.lock
version: 2.0
shards:
  db:
    git: https://github.com/crystal-lang/crystal-db.git
    version: 0.8.0

  sqlite3:
    git: https://github.com/crystal-lang/crystal-sqlite3.git
    version: 0.15.0

Let's suppose I need to work on a fix for crystal-db. I can create a shard.override.yml file pointing to my working copy of crystal-db

# file: shard.override.yml
dependencies:
  db:
    path: /my/path/to/crystal-db

Execute $ shards update to change the installed and locked version of crystal-db.
(Note: if $ shards install is used instead of update, the use will get an error regarding
that db has ambiguous sources due to the existing shard.lock and the shard.override.yml)

$ shards update
Resolving dependencies
Fetching https://github.com/crystal-lang/crystal-sqlite3.git
Installing db (0.9.0 at /my/path/to/crystal-db)
Using sqlite3 (0.15.0)
Writing shard.lock

After this update you will have a symlink to the /my/path/to/crystal-db

$ ls -la lib/
total 8
drwxr-xr-x   5 bcardiff  staff  160 Jul 22 11:15 .
drwxr-xr-x  15 bcardiff  staff  480 Jul 22 11:14 ..
-rw-r--r--   1 bcardiff  staff  196 Jul 22 11:15 .shards.info
lrwxr-xr-x   1 bcardiff  staff   43 Jul 22 11:15 db -> /my/path/to/crystal-db
drwxr-xr-x  12 bcardiff  staff  384 Jul 22 11:14 sqlite3

And the shard.lock has some comments to clarify if the lock was computed with overrides and will not be safe to commit.

# file: shard.lock
# NOTICE: This lockfile contains some overrides from shard.override.yml
version: 2.0
shards:
  db: # Overridden
    path: /my/path/to/crystal-db
    version: 0.9.0

  sqlite3:
    git: https://github.com/crystal-lang/crystal-sqlite3.git
    version: 0.15.0

Note that in this example we also bump crystal-db from 0.8 to 0.9 despite the fact that crystal-sqlite3 requires ~> 0.8.0. So we are overriding the requirements along the dependency graph.

You can override to a specific version, branch, fork, local path and solve ambiguos reference in case the ecosystem do not agree what is the main fork for a shard name.

Having a shard.master.yml will all dependencies to the master/develop branch will simplify how to check if you the project is up to date with non-released changes of dependencies.


Closes #412
Closes #105
Closes #299

@jhass
Copy link
Member

jhass commented Jul 23, 2020

Should we really touch the regular lock opposed to creating a shard.override.lock or not writing down the overrides at all?

If I have to take care to not commit the dirty lock, why have a separate file for overrides in the first place? It could just be section in shard.yml then, there's no difference to partially committing one or two files.

OTOH what's wrong with committing the overrides file and the lock changes from that? If collaborating on a project with outdated dependencies (the primary usecase for this), why should everybody have to redo the overrides?

@Sija
Copy link
Contributor

Sija commented Jul 23, 2020

This whole flow with additional file seems wrong to me. I'd rather take inspiration from Elixir (already mentioned) instead, KISS plz.

@bcardiff
Copy link
Member Author

@jhass in theory there is nothing wrong with committing the override and lock with overrides. That should be done if the overrides are required for an app to work. It happens that I have more present the workflow of temporal overrides to fix things.

If the story for sharing overrides, or forcing some resolution without warnings is wanted the last proposal of the proposal in #412 can be implemented later:

The only scenario where a forced dependency should be tracked is on application shard.yml. In this case, dealing with addition shard.override.yml seems like a reasonable compromise.

If we want overrides to be in the shard.yml we would need an overrides: or resolutions: sibling to dependencies:.

# shard.yml 
dependencies:
  bar:
    github: bar/bar
resolutions:
  foo:
    github: non-main-author/foo

This is similar to what is done in yarn. It would offer a slightly cleaner solution to the scenario where a team needs to share overrides. Here, there would be no need to add a warning in the shard.lock file. If wanted, this extension can be introduced later.

I think that having another lock file will generate confusion. The lock can be used as the source of truth for reinstalling the dependencies, if there are two, then there is a logic need to choose.

@Sija in #412 I mention that adding a forced or override attribute similar to Elixir one is not backward compatible and we will be forced to add a version in the shard.yml. The alternative is to override as in Yarn which I mention. Having an additional files allows per-user customization similar to Bundler which I've found convenient to work with custom forks and local paths.

The logic in requirement_satisfied_by? was unable to allow bumping a nested dependency beyond the main dependency requirement
Advertise override file location in the comment of the lockfile
and do not require the user to perform update when changing the override file.
@bcardiff bcardiff added this to the v0.12.0 milestone Jul 28, 2020
shards install is conservative enough regarding nested dependencies
These were added before the changes in dependencies_for and this would lead to unable to satisfy the lock if there is a new commit in the overrides branch
src/molinillo_solver.cr Outdated Show resolved Hide resolved
src/molinillo_solver.cr Outdated Show resolved Hide resolved
Brian J. Cardiff and others added 3 commits July 30, 2020 14:34
Co-authored-by: Juan Wajnerman <juan.wajnerman@gmail.com>
Handle unlock from version to branch on install (with or without override)
Properly handle locks on overrides branch
Unify unlocking of nested dependencies behavior
@waj waj merged commit e43b954 into crystal-lang:master Jul 31, 2020
@bcardiff bcardiff deleted the feature/overrides branch August 5, 2020 14:19
@mloughran
Copy link

This is a very useful feature but it's currently not easy to find via google – I arrived here by following a trail of issues.

This may be related to the shards command page being outdated and not linked to the man page? /cc crystal-lang/crystal-book#476

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Shards overrides Allow overriding dependencies Local overrides for shards development
5 participants