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

Add MSVC to CI #183

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Add MSVC to CI #183

merged 4 commits into from
Sep 19, 2023

Conversation

jonahbeckford
Copy link
Contributor

These are the commits after following https://github.com/diskuv/dkml-workflows#configure-your-project

I've set the GitHub jobs to continue-on-error: true because MSVC does not work with mirage-crypto. So the jobs won't fail, but at least you can inspect the logs to see what is breaking MSVC.

This goes along with the old #137, although at this point I don't know what else is a problem for mirage-crypto with Windows or MSVC.

@hannesm
Copy link
Member

hannesm commented Sep 18, 2023

thanks for your PR. I'm a bit puzzled to include so much code and binaries into the repository, and wonder what the maintenance story behind that should be? Or do the binaries never need to be updated?

@jonahbeckford
Copy link
Contributor Author

Now I'm confused. When you say "... binaries into the repository", what binaries are you referring to? Are you referring to the shell scripts?

If you are referring to shell scripts, the maintenance story is:

  1. ./dk dkml.wrapper.upgrade will upgrade the scripts ./dk, ./dk.cmd and cmake/FindDkToolScripts.cmake. You should never need to do that unless I make a major version upgrade or a security announcement.
  2. The other 4 scripts can be recreated with ./dk dkml.workflow.compilers CI GitHub Desktop OS Windows, but only if the scripts are erased first. I anticipate you would only want to do that with .github/workflows/dkml.yml, so those same instructions are explicitly listed inside that file.

Finally ...

The bulk of the GitHub logic is downloaded on-demand, so that is always up-to-date (confer: run: ./dk dkml.workflow.compilers CI GitHub in .github/workflows/dkml.yml). You don't need to do it on-demand ... you can check those generated files into source control (effectively pinning the GitHub logic).

Similarly, the two .ci/dkml-compilers/pc scripts do not need to be committed ... you can tell anyone who is developing/testing on their Windows desktops to first run ./dk dkml.workflow.compilers CI Desktop OS Windows. That would have the side-benefit of always being up-to-date, but of course it would be less repeatable for fellow developers.

The "HELP" command listed in .github/workflows/dkml.yml is a good reference as well.

@hannesm
Copy link
Member

hannesm commented Sep 18, 2023

I was refering to BIN +220 KB
.ci/dkml-compilers/pc/setup-dkml-windows_x86.ps1

and BIN +220 KB
.ci/dkml-compilers/pc/setup-dkml-windows_x86_64.ps1

But, as you say, they are text files... :)

Thanks a lot, could you rebase onto the main branch, I'll rerun the CI, and merge?

@jonahbeckford
Copy link
Contributor Author

jonahbeckford commented Sep 18, 2023

Interesting. I'll change the .gitattributes re-encode with a BOM to see if I can stop GitHub from thinking they are binary files (yep, that works).

Then I'll rebase

The ./dk script downloads other DkML
scripts. We want it to download the CI
scripts.
Ran: ./dk dkml.workflow.compilers CI GitHub Desktop OS Windows
@hannesm
Copy link
Member

hannesm commented Sep 19, 2023

thanks a lot

@hannesm hannesm merged commit 1450431 into mirage:main Sep 19, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants