-
Notifications
You must be signed in to change notification settings - Fork 460
Write commit hashes into the lock dir when locking git repositories #13297
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
base: main
Are you sure you want to change the base?
Write commit hashes into the lock dir when locking git repositories #13297
Conversation
d669f81 to
64a79d3
Compare
src/dune_pkg/lock_pkg.ml
Outdated
| (match url.backend with | ||
| | `git -> | ||
| (match Resolved_package.at_rev resolved_pkg with | ||
| | None -> url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case happens when it is a dune pin or it is a files directory opam pin. Is that what we want here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't work out how this can be None, if it is really impossible we should Code_error instead of falling back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a Code_error.raise but the tests in opam-source-conversion.t are failing. Need to investigate this, because they might be resolving the packages incorrectly as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so this branch triggers when this is happening in opam-source-conversion.t:
$ mkpkg testpkg <<EOF
> url {
> src: "git+http://github.com/foo"
> checksum: "md5=069aa55d40e548280f92af693f6c625a"
> }
> EOF
This is because the URL is a Git URL, but the resolved package testpkg actually comes from the file system (the mock repo), thus it is loaded via Resolved_package.local_fs which does not load an At_rev.t (and should not need to).
It seems like relying on the Resolved_package to read the revision from is wrong in the general case, as the Resolved_package only matches the source when pinning or vendoring but does not when the package is loaded from an OPAM repo (at which point the Resolved_package is in the OPAM repo and completely disconnected from the actual source).
Signed-off-by: Marek Kubica <[email protected]>
Signed-off-by: Marek Kubica <[email protected]>
Signed-off-by: Marek Kubica <[email protected]>
Signed-off-by: Marek Kubica <[email protected]>
Alizter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add test cases for branches and tags?
Signed-off-by: Marek Kubica <[email protected]>
64a79d3 to
c3fbe74
Compare
Signed-off-by: Marek Kubica <[email protected]>
This PR applies #13176 and adds the remaining code to implement the behavior discussed in #13110. The tests change accordingly with the changed semantics of
gitURLs.Closes #13110