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

opam export: record vcs commit hash when a vcs url is specified #4055

Merged
merged 5 commits into from Mar 10, 2020

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Jan 7, 2020

this is useful for having an export file which can be imported and will contain the exact same sources (related to #4040), part of reproducibilty efforts

@rjbou
Copy link
Collaborator

@rjbou rjbou commented Jan 10, 2020

Agree to integrate this sspecification, to be sure to rebuild the exact same switch. There is a side effect: I change pinned packages (and some from repo acceptiong allowing vcs urls): e;g if you pin a package to a branch, after an export & import this package will be pinned to an exact commit. There is a way to retrieve the commit branch (it's implemented in opam or a plugin), but we loose the information if first user wanted to pin to a commit or a branch.

src/client/opamSwitchCommand.ml Outdated Show resolved Hide resolved
src/client/opamSwitchCommand.ml Outdated Show resolved Hide resolved
src/client/opamSwitchCommand.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Show resolved Hide resolved
@rjbou rjbou added PR: WIP PR: NEEDS UPDATE labels Jan 24, 2020
@hannesm hannesm changed the title opam export: record git commit hash when a git url is specified opam export: record vcs commit hash when a vcs url is specified Jan 27, 2020
@hannesm
Copy link
Member Author

@hannesm hannesm commented Jan 27, 2020

I rebased on top of master and added a commit which addresses the review comments. when merging #4040 (that now adds an optional labeled argument freeze to OpamSwitchCommands.export) the code from this PR should only be executed if freeze is provided and true.

@hannesm
Copy link
Member Author

@hannesm hannesm commented Mar 5, 2020

now that #4040 is merged, I rebased this on top of master - and by doing this I squashed all commits (which simplified the rebase). I also guarded, as discussed some weeks ago, this behaviour behind an optional labeled freeze argument to export.

apart from this PR, only df3bd35 and 365ac02 are needed for orb. I can cherry-pick them on here if that is preferable.

thanks for reviews, discussion, and merges :)

src/client/opamSwitchCommand.ml Outdated Show resolved Hide resolved
@rjbou
Copy link
Collaborator

@rjbou rjbou commented Mar 6, 2020

I updated few things:

  • add freeze (that implies full) as a opam switch export option
  • changes warnings into errors
  • use [Opamrepository.revision] to handle other VCSs
  • use [OpamFile.URL.with_url] to not recreate an url
  • fix source dir look up
    @hannes, can you check that this work on your tests?

@hannesm
Copy link
Member Author

@hannesm hannesm commented Mar 9, 2020

thanks @rjbou -- indeed this works fine for me. tested with:

$ opam pin add orb https://github.com/hannesm/orb.git#active # this includes this branch and the two commits mentioned above on the opam side
$ opam repo add test-unikernels https://github.com/roburio/reproducible-unikernel-repo.git
$ orb orb build --twice --repo=test-unikernels,default unikernel-hello-hvt

and then the resulting /tmp/bi-unikernel-hello*/*opam-switch contains ocamlgraph and unikernel-hello-hvt which are pinned to the respective git commits.

@rjbou rjbou added PR: LGTM and removed PR: NEEDS UPDATE PR: WIP labels Mar 9, 2020
rjbou
rjbou approved these changes Mar 9, 2020
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamSwitchCommand.ml Outdated Show resolved Hide resolved
src/client/opamSwitchCommand.ml Outdated Show resolved Hide resolved
Copy link
Member

@AltGr AltGr left a comment

Some typos, otherwise LGTM

src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamSwitchCommand.ml Show resolved Hide resolved
src/client/opamSwitchCommand.ml Show resolved Hide resolved
hannesm and others added 5 commits Mar 9, 2020
* changes warnings into errors
* use [Opamrepository.revision] to handle other VCSs
* use [OpamFile.URL.with_url] to not recreate an url
* fix source dir look up
@rjbou rjbou merged commit 13bc423 into ocaml:master Mar 10, 2020
2 checks passed
@hannesm hannesm deleted the rewrite-git branch Mar 10, 2020
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.

None yet

3 participants