-
Notifications
You must be signed in to change notification settings - Fork 399
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 GitHub Actions configuration file #3451
Conversation
45e6dfb
to
f2a80b2
Compare
Thanks for playing with github actions for dune :) Would this replace Travis and AppVeyor? |
Yes, that's right. and this works without complex shell scripts. |
By the way, you can check here that it works correctly. |
Nice! No reason for a short period not to run it alongside AppVeyor/Travis and then retire them a few weeks after merging? |
I'm not against it at all, but this is because the complex logic is offloaded to another action! Another important caveat at the moment is run time - part of the complexity (especially on Travis) is managing the caching. The action is presently taking half an hour to run where the AppVeyor takes 90 seconds and Travis around 17 minutes. |
Run time is definitely a concern. If there are other benefits to switching to github actions, we can accept a small slowdown, but going from 17 minutes to half an hour is too much. |
The run time increase is worrying and bit unexpected. When we switched lsp to github actions from travis, our run times improved significantly. |
Maybe it's because of the caching @dra27 described. Do you do anything clever in lsp regarding opam caching? |
I don't think we have any special caching (@giltho can confirm). But we also don't install nearly as much dependencies for tests. |
eb5eeae
to
05c385a
Compare
Yeah, you're absolutely right! My (and our?) goal is to push the many complex tasks that are going on in each repository to a particular Action. |
For example, I've been working for about 6 months recently to speed up I also plan to rewrite the |
1a5f911
to
052f9fe
Compare
Yep confirming that we have no caching at all in ocaml-lsp's CI The main advantage of moving to gha is indeed to have all the complexity encapsulated in an action instead of having custom scripts. Dune is a major engine that moves the community forward and it would be very nice for it to use gha so that the action can improve, and therefore improve for the whole community :) That being said, it would probably be better to wait imo for the caching to work before merging this, or at least not remove the rest of the CI and make the gha checks optional for merging. |
ad29def
to
967602b
Compare
I'm sorry for committing so much. I was caught in some traps. I think I was able to reflect all suggestions in code review. |
No worries, we'll probably just squash everything before merging. Committing a lot during development is generally good practice :) Otherwise, I'm personally happy to move to github actions. It's nice to have all the platforms tested in one place. But the run time is definitely a concern, and we should indeed wait until it is close enough to the current run time we get with the combo Travis/AppVeyor. |
Thank you for your kind words. Yes, it makes sense. I will mark this PR as draft until their speed improves! |
I looked at the CI configuration and it doesn't seem like the run times are comparable. Here are some differences:
In summary, no wonder github actions seems a lot slower -- we are testing quite a bit more configurations. I would suggest the following changes:
|
Once #3466 is merged, we can drop installing gstat with homebrew by the way. |
Running jobs on all platforms and versions are done in parallel, so I don't think it will speed up CI. |
If we want to speed everything up, we need to set up the OCaml environment within seconds to a minute. Because GitHub Actions has a cache limit for users, at least for now.
But, on the Action side is unlimited. So I think we can have the pre-built switch on the setup-ocaml side. And that's what I'm working on now. |
Signed-off-by: Sora Morimoto <git@bsky.moe>
Signed-off-by: Sora Morimoto <git@bsky.moe>
Signed-off-by: Sora Morimoto <git@bsky.moe>
Signed-off-by: Sora Morimoto <git@bsky.moe>
Signed-off-by: Sora Morimoto <git@bsky.moe>
@rgrinberg - the important figure, which his comparable is "How long does it all take"... reducing the testing matrix is a fix for that, being able to parallelise is another! 🙂 |
It works for small projects, but the problem is projects like this. The cache limit is only 5 GB and will likely result in cache thrashing which will not get the expected results. |
Ok I see, thanks for you answer ! (Apparently I accidentality deleted my question: I am using |
@imbsky are there any updates on the caching issue? we've discussed this PR at the recent dune meeting and there was renewed interest in it. |
I started a personal experiment project like this. Hopefully this will reduce the compilation time of the compiler itself. I want to work on this project as soon as I get home after this week's exams and so on. |
To make it faster, I want to work more closely with the GitHub team rather than doing it in my own way, and while the project above is a bit stalled, things are moving forward. |
That's good to hear :) We look forward to a simpler and more centralised CI, the current one based on Travis and AppVeyor is regularly causing us pain. |
Replaced by #3712. |
This PR adds a configuration file for GitHub Actions.
There are jobs that don't build and/or test for a variety of reasons, but they are all commented out. See the diagram below for what to do on which version and platform.