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

Resolve to the "nearest" version #108

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Resolve to the "nearest" version #108

merged 1 commit into from
Oct 30, 2023

Conversation

rtimush
Copy link
Collaborator

@rtimush rtimush commented Oct 30, 2023

Previously, when a dependency tree contained multiple versions of the same dependency we used the following rules to resolve the conflicts:

  1. Prefer lexicographically greater revisions.
  2. If there is a branch specification and a revision specification, try to use both and fail if the revision tag is not reachable from the specified branch head.

There are a few problems with these rules:

  • Rule 1 does not give user much control over transitive dependency versions. You can only override to a bigger revision.
  • Lexicographic comparison does not necessarily make sense for revision strings.
  • Rule 2 does not work with some git workflows.
  • There are some corner cases when transitive dependencies change depending on the version we choose.

I'd say the first problem is the most important one, as it makes it impossible to work around other.

This PR implements an alternative conflict resolution strategy — the dependencies nearest to the root win. This is, for example, similar to what Maven does. This makes it trivial to override any dependency by explicitly specifying the revision in the protofetch.toml.

Besides making version resolution more flexible (which I suppose is rarely a problem in practice, as dependency trees are usually relatively small), there is a second reason why I'd like to make this change. With the current rules, transitive dependencies affect the dependency resolution but are not recorded in the lock file, so while the lock file stores the resulting revisions, it is impossible to verify that the lock file corresponds to protofetch.toml. An alternative solution could be to write all transitive dependencies in the lock file, even if they are not a part of the final dependency set. This is however not backwards compatible, and in my opinion it'd be nice to changing the resolution strategy anyway.

@rtimush rtimush requested a review from a team October 30, 2023 10:30
@kolov
Copy link

kolov commented Oct 30, 2023

Why is it important to be able to "verify that the lock file corresponds to protofetch.toml"?

@rtimush
Copy link
Collaborator Author

rtimush commented Oct 30, 2023

@kolov This is related to #83. If you edit the protofetch.toml file, we should be able to detect this and update the lock file (ideally, only the parts that correspond to the toml file changes). This is how all other tools that have lock files work, but with protofetch you have to explicitly update the lock file with fetch -f, otherwise, the changes in the toml file are simply ignored.

@rtimush rtimush merged commit e24f74a into master Oct 30, 2023
13 checks passed
@rtimush rtimush deleted the resolve-closest branch October 31, 2023 14:08
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.

2 participants