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

Update ocaml/setup-ocaml to v3 #1153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smorimoto
Copy link
Member

No description provided.

@panglesd panglesd added the no changelog This pull request does not need a changelog entry label Jul 2, 2024
@panglesd
Copy link
Collaborator

panglesd commented Jul 2, 2024

If I understand correctly the setup-ocaml release changelog, this setups ocaml with opam 2.2 for windows support?
Would that enable windows CI testing? That would be great!

@smorimoto smorimoto marked this pull request as draft July 2, 2024 11:45
@smorimoto
Copy link
Member Author

I added Windows, but it seems to fail due to the path delimiter, etc.

@hhugo
Copy link

hhugo commented Jul 3, 2024

You shouldn't use Fpath.to_string when generating path in dune files.

Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

I think we are not ready yet to add windows testing on our CI. However, it is very nice to be able to add it so easily!

I think this is good to merge as soon as the windows-triggering line is commented out. We'll uncomment it after fixing the windows issues.

Thanks a lot for your work!

.github/dependabot.yml Show resolved Hide resolved
@panglesd panglesd mentioned this pull request Jul 22, 2024
@smorimoto smorimoto force-pushed the setup-ocaml-v3 branch 2 times, most recently from 6e7cbaf to dbfca79 Compare August 5, 2024 18:31
Signed-off-by: Sora Morimoto <[email protected]>
@smorimoto smorimoto marked this pull request as ready for review August 5, 2024 20:12
@smorimoto
Copy link
Member Author

Ready to go!

Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

Thanks and sorry for the slow review: summer if full of holidays!

@@ -1,4 +1,4 @@
(lang dune 3.7)
(lang dune 3.16)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should raise the minimal version in odoc.opam file!

@@ -1,3 +1,3 @@
module-item-spacing=preserve
version=0.26.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should do that at some point but in a separate PR

Comment on lines -15 to +18
- 4.02.x
- 4.04.x
- 4.06.x
- 4.08.x
- 4.10.x
- 4.12.x
- 4.14.x
- 5.0.x
- 5.1.x
- 4
- 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, this runs odoc's test in 4.02 on ubuntu latest (due to the include below), and on 4.14 and the latest 5 version for linux, windows and mac.

I think it makes sense for odoc to continue checking each individual versions (4.07, 5.0 and 5.1, ...). Indeed, odoc has a lot of compatibility code for instance for the different ASTs or for the shapes, that can easily break so that we want to continue testing. (This compatibility code might work for 4.02 and 4.14 but not for an intermediate versions).


- name: Install dependencies
run: opam install -y --deps-only -t ./odoc.opam ./odoc-parser.opam
run: opam install --deps-only --with-test odoc odoc-parser
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't that install deps from the odoc from opam repository in case where run-mdx is true, breaking if we modify the dependencies without releasing?

Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

Being able to run windows test without failing the CI is nice, but a little bit of a resource waste until we start fixing it. Maybe it would be better to disable it until we start working on #1169 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This pull request does not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants