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

[core] [api] Rename Int and Option modules. #10469

Closed
wants to merge 1 commit into from

Conversation

ejgallego
Copy link
Member

@ejgallego ejgallego commented Jul 3, 2019

OCaml 4.08.0 has introduced Int and Option modules, we thus rename
Coq's to CInt and COption to avoid problems with upstream.

@ejgallego ejgallego added the kind: compatibility Changes allowing for compatibility between versions. label Jul 3, 2019
@ejgallego ejgallego added this to the 8.11+beta1 milestone Jul 3, 2019
@ejgallego ejgallego added the kind: infrastructure CI, build tools, development tools. label Jul 3, 2019
@ejgallego ejgallego removed the needs: merge of dependency This PR depends on another PR being merged first. label Jul 9, 2019
@SkySkimmer
Copy link
Contributor

SkySkimmer commented Jul 9, 2019 via email

@ejgallego ejgallego force-pushed the ocaml+4.08_redux branch 3 times, most recently from b7fabbc to e737766 Compare July 12, 2019 23:08
@SkySkimmer SkySkimmer added needs: overlay This is breaking external developments we track in CI. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. labels Jul 23, 2019
@SkySkimmer
Copy link
Contributor

So are we doing this or not?

@ejgallego
Copy link
Member Author

So are we doing this or not?

Main motivation for me to do this is ocaml/merlin#1003 , but I'm unsure about what the right behavior is here and I didn't have to time to look into it too much, so kinda waiting for the merlin people to comment.

@coqbot coqbot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Aug 15, 2019
@XVilka
Copy link

XVilka commented Oct 6, 2019

There are many conflicts now.

@ejgallego
Copy link
Member Author

Rebasing is trivial, but before doing it; I wonder: what should we do with this PR?

The PR is targeted at 8.11 beta, I dunno what to do, we never set a formal policy about this, but it has been the norm in the past than whenever OCaml does introduce some new modules we rename our conflicting ones. [we should be able to stop doing this once we can pack our modules]

@ppedrot
Copy link
Member

ppedrot commented Oct 31, 2019

Judging from the lack of reaction I propose postponing to 8.12.

@ppedrot ppedrot modified the milestones: 8.11+beta1, 8.12+beta1 Oct 31, 2019
@ejgallego
Copy link
Member Author

ejgallego commented Oct 31, 2019

Judging from the lack of reaction I propose postponing to 8.12.

I guess we can just close this if we don't merge; I don't see the point on having this on 8.12 but not on 8.11. On the other hand we should check we are not having any double linking in 4.08.0 / 4.09.0 [unsure because of the upstream packing]

If that's the case this patch must land in 8.11

@ejgallego
Copy link
Member Author

On the other hand we should check we are not having any double linking in 4.08.0 / 4.09.0 [unsure because of the upstream packing]

If that's the case this patch must land in 8.11

After some checking things seem to be fine; so this patch is not a must.

On the other hand it may be desirable for other reasons, for example merlin has a bug that will get it confused without this patch [1], so I did a last rebase just in case we want to ship it in the last minute.

[1] ocaml/merlin#1003

OCaml 4.08.0 has introduced `Int` and `Option` modules, we thus rename
Coq's to `CInt` and `COption` to avoid problems with upstream.
@ejgallego
Copy link
Member Author

In the end we have decided that this is overkill, as the upstream modules are already packed.

@ejgallego ejgallego closed this Dec 11, 2019
@coqbot coqbot removed this from the 8.12+beta1 milestone Dec 11, 2019
@ejgallego ejgallego deleted the ocaml+4.08_redux branch December 11, 2019 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: compatibility Changes allowing for compatibility between versions. kind: infrastructure CI, build tools, development tools. needs: overlay This is breaking external developments we track in CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants