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

[ci] [gh-actions] Run the platform job using Github Actions #12425

Merged
merged 4 commits into from Sep 10, 2021

Conversation

ejgallego
Copy link
Member

@ejgallego ejgallego commented May 29, 2020

We move the CI job creating the platform windows installer from Gitlab-driven Inria custom runners to standard Github actions one.

While we are at it, we significantly cleanup the build script, trying to avoid as much hackery as possible, and splitting it in logical stages.

Notwithstanding, the current system is still not very satisfactory, in particular:

  • build time is very long, we should cache cygwin folder at least, and only rebuild if the coq-platform ref we use has changed; fortunately GH actions do support this
  • verbosity of the script is out of control
  • we can still cleanup way more things

Please review the list of chosen bugs below, and speak if you think it is too wide.

closes #6807 , closes #7428 , closes #8046 , closes #8622 , closes #9401 , closes #11073

Kind: infrastructure.

  • Added / updated test-suite
  • Corresponding documentation was added / updated
  • Entry added in the changelog

Note: this PR was originally started from the nice setup of Jason Gross [1] for windows CI setup, but it was updated to actually use the platform after #13598

[1] https://github.com/mit-plv/fiat-crypto/blob/master/.github/workflows/coq-windows.yml

@ejgallego ejgallego requested a review from JasonGross May 29, 2020 13:40
@ejgallego ejgallego added kind: infrastructure CI, build tools, development tools. platform: Windows This is a Windows specific issue. labels May 29, 2020
@ejgallego ejgallego force-pushed the ci+gh_actions_win branch 3 times, most recently from 55081f9 to 5c098f5 Compare May 29, 2020 13:49
@ejgallego

This comment has been minimized.

@JasonGross

This comment has been minimized.

@ejgallego

This comment has been minimized.

@ejgallego

This comment has been minimized.

@ejgallego ejgallego force-pushed the ci+gh_actions_win branch 2 times, most recently from e54c621 to 1f6aa1f Compare May 29, 2020 15:04
@ejgallego ejgallego force-pushed the ci+gh_actions_win branch 6 times, most recently from 6048ec2 to ec773f0 Compare May 29, 2020 17:34
@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Aug 24, 2020
@coqbot-app coqbot-app bot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Mar 29, 2021
@gares

This comment has been minimized.

@Zimmi48

This comment has been minimized.

@gares
Copy link
Member

gares commented Mar 30, 2021

Yes, but is this job supposed to test?

@gares

This comment has been minimized.

@gares

This comment has been minimized.

@gares

This comment has been minimized.

@MSoegtropIMC
Copy link
Contributor

@ejgallego : for sure one could add an option to run the test suite. The platform CI runs a very simple "smoke test" - that is one or two selected examples from each provided package. The idea is that Coq Platform is a kind of meta build and relies on that the packages it builds are OK. The idea of the "smoke test" kit is to check if e.g. installation of packages went fine.

@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Sep 7, 2021
@coqbot-app coqbot-app bot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Sep 7, 2021
@ejgallego
Copy link
Member Author

@MSoegtropIMC , another question, how difficult would it be to split the cygwin setup to its own script?

This way we could run it in its separate step, and have an easier time caching.

Similarly for opam, would be very interesting to separate opam install --deps-only coq from the actual opam install coq

Caching for opam should work fine and be reproducible as soon as you folks start using opam lock files.

@MSoegtropIMC
Copy link
Contributor

@ejgallego : this is already supported:

  • pass -build=n to coq_platform_make_windows.bat - this will just setup cygwin but do not run the script
  • start up the generated cygwin and run coq_platform_make.sh from the copied coq_platform folder or alternatively git clone something else and start the script.

But I must admit, this is not tested much and also the cygwin setup sometimes changes.

@ejgallego
Copy link
Member Author

But I must admit, this is not tested much and also the cygwin setup sometimes changes.

Thanks for the feedback, will try!

I guess the way to make this more reliable would be to structure the platform build as a sequential call of these scripts from a top-level driver.

@@ -0,0 +1,11 @@
- **Changed:**
The Windows installer CI build has been moved from custom workers to
Copy link
Member

Choose a reason for hiding this comment

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

That's not something that we typically document in the user-facing changelog (which is already long enough without these kinds of details). It would make more sense in the dev changelog.

Copy link
Member Author

Choose a reason for hiding this comment

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

As you prefer, I was under the impression that some users were downloading the windows artifacts for master, so I signaled it.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. Maybe reformulate to put this aspect forward (and maybe do not link to the fixed issues which are not really relevant for users).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I did tweak it.

.github/CODEOWNERS Show resolved Hide resolved
We move the CI job creating the platform windows installer from
Gitlab-driven Inria custom runners to standard Github actions one.

While we are at it, we significantly cleanup the build script, trying
to avoid as much hackery as possible, and splitting it in logical
stages.

Notwithstanding, the current system is still not very satisfactory, in
particular:

- build time is very long, we should cache cygwin folder at least, and
  only rebuild if the coq-platform ref we use has changed; fortunately
  GH actions do support this
- verbosity of the script is out of control
- we can still cleanup way more things

Please review the list of chosen bugs below, and speak if you think it
is too wide.

closes coq#6807 , closes coq#7428 , closes coq#8046 , closes coq#8622 , closes #coq#9401 , closes coq#11073

Note: this PR was originally started from the nice setup of Jason
Gross [1] for windows CI setup, but it was updated to actually use the
platform after coq#13598

[1] https://github.com/mit-plv/fiat-crypto/blob/master/.github/workflows/coq-windows.yml[ci] [gh-actions] Initial setup of Windows Github Build
This is a bit more modular build, additionally

- Quite a few cleanups done, including removing the non-needed cleanup
  and unique folder stuff, and avoiding quoting hacks for the artifact
  generation
- We also try cygwin quiet mode, but without much success
@Zimmi48 Zimmi48 self-assigned this Sep 10, 2021
@Zimmi48
Copy link
Member

Zimmi48 commented Sep 10, 2021

Will merge when CI passes.

@gares can I add you to the @coq/windows-build-maintainers team?

@gares
Copy link
Member

gares commented Sep 10, 2021

no

@Zimmi48
Copy link
Member

Zimmi48 commented Sep 10, 2021

OK, I figured that you were the best person to be in this team (more suited than me anyway) given that you've authored most of the code that is being moved in dev/ci/platform. Maybe we should just remove the @coq/windows-build-maintainers team and leave this under the ownership of @coq/ci-maintainers then.

@gares
Copy link
Member

gares commented Sep 10, 2021

I'm not fluent in .bat, and I don't even have a windows PC at hand...

@Zimmi48
Copy link
Member

Zimmi48 commented Sep 10, 2021

Yeah, me neither. Anyway, let's keep things as they are for the time being.

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 58422f1 into coq:master Sep 10, 2021
@ejgallego ejgallego deleted the ci+gh_actions_win branch September 10, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: infrastructure CI, build tools, development tools. platform: Windows This is a Windows specific issue.
Projects
None yet
6 participants