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

Port to dune 2.0 #1196

Merged
merged 5 commits into from
May 13, 2020
Merged

Port to dune 2.0 #1196

merged 5 commits into from
May 13, 2020

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Dec 18, 2019

This ports to (lang dune 2.0) and uses all niceties from the new dune lang.

  • (with-accepted-exit-codes) & (with-stdin-from) (these replace all our uses of (system))
  • long-form target inference thanks to @NathanReb (this means that we can use file names directly without having to declare (targets) and refer to them as %{targets}

Note that this bumps the minimum dune version to 2.0.0. This shouldn't be too much of a problem since very few packages have a < 2.0.0 constraint on opam-repository (and almost all of them have a fixed version - the compatibility is pretty good).

Let me know what you think.

@jberdine
Copy link
Collaborator

jberdine commented Dec 18, 2019 via email

@gpetiot
Copy link
Collaborator

gpetiot commented Dec 18, 2019

I think it's worth adding a line in the changelog that the required dune version is now >= 2.0.0, otherwise the changes in dune files look good.

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I agree this should be in the changelog.
Compatibility is not necessarily a concern as upgrading OCamlformat itself is involved and I don't expect users to upgrade it as soon as possible.

@emillon
Copy link
Collaborator Author

emillon commented Dec 19, 2019

I agree with the changelog. We can wait a bit until the dune 2 issues are identified.

@XVilka
Copy link
Contributor

XVilka commented Jan 16, 2020

There might be a problem with Dune >= 2.0 migration - some popular packages still depend on the jbuilder (jbuilder.transition) that is incompatible with the newer dune, so you wouldn't be able to install the ocamlformat in the opam switches with these packages installed:

P.S. You might want to rebase your PR.

@jberdine
Copy link
Collaborator

Since jbuilder and dune 2 can be co-installed, aren't those packages that depend on jbuilder.transition instead of jbuilder just broken?

@XVilka
Copy link
Contributor

XVilka commented Jan 16, 2020

@jberdine I am not sure, opam doesn't allow me to upgrade those precisely because of that dependency on jbuilder:

Everything as up-to-date as possible (run with --verbose to show unavailable upgrades).

The following packages are not being upgraded because the new versions conflict with other installed packages:
  - dune.2.1.2
    ∗ jbuilder.transition is installed and requires dune < 2.0
  - dune-configurator.2.1.2

@emillon
Copy link
Collaborator Author

emillon commented Jan 16, 2020

You can still fix these packages, or use an older version of ocamlformat.

The current situation with dependencies is not ideal: since at the moment you have to build ocamlformat in the same switch as your sources, it also induces a constraint on base and other ocamlformat dependencies. But that's not strictly required: a binary is just a binary, it does not depend on dune or base at runtime. It will be a nicer solution if we can fix that at the opam level.

@jberdine
Copy link
Collaborator

jberdine commented Jan 16, 2020 via email

@emillon
Copy link
Collaborator Author

emillon commented Jan 16, 2020

I mean if you rely on opam to install ocamlformat as test-dep or something like that. I agree with you that it's best to do it in a separate switch (that's effectively what ocaml-ci is doing), but there's no way to tell opam to do that for now.

@emillon emillon force-pushed the dune2 branch 3 times, most recently from f877114 to c1b175c Compare April 15, 2020 12:05
@emillon
Copy link
Collaborator Author

emillon commented Apr 15, 2020

I just rebased this; not sure what's going on with the ocp-indent tests. (I tried to force >= 1.8.1 in case of a different version but that did not fix the issue)

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a problem with dune build @fmt. It seems that test/.ocamlformat_ignore wasn't useful and formatting of tests were disabled by something else. Could it be that test/passing/dune-project was overriding (using fmt) (by not specifying it) from ./dune-project ?

The reason test/.ocamlformat_ignore is not working is that it should be called test/.ocamlformat-ignore.

test/passing/gen/gen.ml Outdated Show resolved Hide resolved
@emillon
Copy link
Collaborator Author

emillon commented Apr 20, 2020

Formatting rules were not set up by default as the project was a 1.x. In a 2.x project it's necessary to disable them using (formatting disabled). I fixed this part.

@emillon emillon force-pushed the dune2 branch 2 times, most recently from 55bf97b to e8abe72 Compare April 20, 2020 12:07
@emillon
Copy link
Collaborator Author

emillon commented Apr 20, 2020

and this is working. nice catch, thanks!

Copy link
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After that, I think we can merge !

dune Outdated Show resolved Hide resolved
@emillon emillon added the no changelog set this to bypass the CI check for changelog entries label May 13, 2020
@emillon emillon merged commit 693fcd4 into ocaml-ppx:master May 13, 2020
@emillon emillon deleted the dune2 branch May 13, 2020 10:14
gpetiot pushed a commit to gpetiot/ocamlformat that referenced this pull request May 13, 2020
* Use (lang dune 2.2)

* Use with-accepted-exit-codes

* read lines in generator

* use with-stdin-from

* Use long-form target inference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog set this to bypass the CI check for changelog entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants