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

Add opam files to allow pinning #8604

Merged
merged 5 commits into from Apr 12, 2019

Conversation

Projects
None yet
6 participants
@gretay-js
Copy link
Contributor

commented Apr 10, 2019

Address comments from PR #2207.

Additionally, attached ocaml-4.08.0.opam file should be added to opam repository.

ocaml-4.08.0.opam.txt

lpw25 and others added some commits Dec 19, 2018

Address review comments
#2207 (comment)

Remove ocaml.opam.
Update release notes to change ocaml-variants.opam when VERSION changes.
Add .gitattribute prune.typo for ocaml-variants.opam.

@gretay-js gretay-js force-pushed the gretay-js:pr2207 branch from a0f4a7f to 9000f93 Apr 10, 2019

@gretay-js gretay-js force-pushed the gretay-js:pr2207 branch from 9000f93 to e9b2f6f Apr 10, 2019

----
opam switch create . --empty
opam install .
----

This comment has been minimized.

Copy link
@Drup

Drup Apr 11, 2019

Contributor

Why advise a local switch for this ?

This comment has been minimized.

Copy link
@gasche

gasche Apr 12, 2019

Member

Why not? One benefit I see is that "get rid of all this stuff" operations such as git clean -xfd will also get rid of the switch.

@@ -0,0 +1,30 @@
opam-version: "2.0"
name: "ocaml-variants"
version: "4.08.0+dev"

This comment has been minimized.

Copy link
@Drup

Drup Apr 11, 2019

Contributor

The usual name for ocaml dev channel is "trunk"

@@ -0,0 +1,30 @@
opam-version: "2.0"
name: "ocaml-variants"

This comment has been minimized.

Copy link
@Drup

Drup Apr 11, 2019

Contributor

You don't need the name here.

]
install: [make "install"]
maintainer: "caml-list@inria.fr"
homepage: "https://ocaml.org"

This comment has been minimized.

Copy link
@Drup

Drup Apr 11, 2019

Contributor

Given this is the dev version, I think putting the github page here is more appropriate.

This comment has been minimized.

Copy link
@gretay-js

gretay-js Apr 12, 2019

Author Contributor

Fixed this and the other comments above.

version: "4.08.0+dev"
synopsis: "OCaml development version"
depends: [
"ocaml" {= "4.08.0" & post}

This comment has been minimized.

Copy link
@Drup

Drup Apr 11, 2019

Contributor

IIRC, we can put = version here, and it'll do the right thing directly.

This comment has been minimized.

Copy link
@gretay-js

gretay-js Apr 12, 2019

Author Contributor

I have just tried it with depends: [ "ocaml" {= "%{version}%" & post} ...
It fails when I try to create a local repo from a global system switch that has 4.07.1.

The following dependencies couldn't be met:
  - ocaml-variants -> ocaml >= 4.08.0+trunk
      no matching version

Is this the behavior you expect? With the explicit dependency on 4.08.0 & post, opam finds ocaml.opam (currently in the same directory) and installs ocaml-variants successfully.

This comment has been minimized.

Copy link
@Drup

Drup Apr 12, 2019

Contributor

Ah, wait, the version is 4.08.0+trunk, but we want 4.08.0. Nevermind, disregard that remark. :)

@Drup

Drup approved these changes Apr 12, 2019

Copy link
Contributor

left a comment

Looks good!

Changes Outdated
@@ -582,6 +582,9 @@ OCaml 4.08.0
* GPR#8533: Remove some unused configure tests
(Stephen Dolan, review by David Allsopp and Sébastien Hinderer)

- GPR#2207,#8604: Add opam files to allow pinning
(Leo White, Greta Yorsh)

This comment has been minimized.

Copy link
@gasche

gasche Apr 12, 2019

Member

@gretay-js please include Gabriel Radanne as a reviewer and I'll merge. Thanks both (and Leo) for the work!

@gretay-js

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@gasche gasche merged commit e89287f into ocaml:4.08 Apr 12, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

I just merged, and this went into the 4.08 branch. If I understand correctly, I should update the version to 4.09 to cherry-pick in trunk, but this also requires adding a 4.09 version in opam-repository or using ocaml-beta-repository in the meantime.

@gretay-js

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

gasche added a commit that referenced this pull request Apr 12, 2019

Merge pull request #8604 from gretay-js/pr2207
Add opam files to allow pinning

(cherry picked from commit e89287f)
(version changed 4.08 => 4.09)
@gasche

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

I cherry-picked in trunk, but I didn't do anything on the opam-repository side. Anyone willing to prepare a change there is welcome, otherwise I'll get to it at some (unknown) later point.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Note that the ocaml-variants package on trunk doesn't want updating at release it wants updating at branch time cf #2207 (comment).

It's always OK to have ocaml packages in opam-repository for unreleased versions - at this moment, ocaml.4.08.0 and ocaml.4.09.0 could (and should) both be there. The key thing at the moment is that there must never be an ocaml-variants package in opam-repository which has a version greater than the latest ocaml-base-compiler in opam-repository.

So, when we branch 4.09, ocaml-variants.opam on trunk should immediately be updated to be 4.10 and ocaml.4.10.0 should be merged to opam-repository.

@avsm

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

@dra27 Could we maybe revisit the decision to not include an ocaml.opam file? At the moment, pinning the compiler seems to work approximately none of the time because the opam-repository is not updated when the VERSION file is changed in a branch. For example, both trunk and 4.08 branches are currently on versions that I don't think have ocaml opam files in the repository -- and in the case of the 4.08 branch it is not even clear that the version should be added to the repository.

Is there any actual harm in keeping a local copy in the distribution? Maybe with a script to update the versions in it based on the VERSION file so it is easy to fix when things are changed (or even to check in CI that these things are in sync).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.