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 continuous testing on Windows using GitHub Actions #8676

Merged
merged 5 commits into from Jan 29, 2020

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Jan 11, 2020

This is a pipeline using a Linux job that cross-compiles for Windows, followed by an actual Windows job linking and running that.

Based on https://github.com/crystal-lang/crystal/wiki/Porting-to-Windows/30956f45a4e739204881694ee6c92f4fb0bc5395

Note that all the actual commits from this PR are prefixed with "Win CI:", the rest is merges of pull requests that this relies on.
This PR creates and touches only the file ".github/workflows/win.yml".

Example in action

Fixes #8666.

@straight-shoota
Copy link
Member

This is really amazing! I almost didn't expect to get CI working on windows so quickly.

A few remarks:

  • In order to provide the same behaviour as the existing CI workflows, it would be nice if we could build stdlib specs with both latest and master compiler. I.e. before and after the build crystal step.
  • The versions for libgc and libatomic_ops in the guide are outdated. I didn't test it yet, but I suppose the latest releases should work as well. That's v8.0.4 and v7.6.10 as used for the linux compiler build.
  • I haven't looked deeply into GitHub actions but I figure we could extract building the library into a re-usable action step? I think that would help to separate concerns and keep the CI config more clear. Alternatively, a separate PowerShell script would also work and even provide the benefit that it would be easily applicable for local installs.

@oprypin
Copy link
Member Author

oprypin commented Jan 12, 2020

  1. Sure, that can be done as a follow-up if deemed necessary. To me it seems like it's just extra runtime for no benefit currently. It's being checked on other systems, and the Windows build doesn't satisfy even weaker guarantees than this one.
  2. Yes, but note that these versions are newer than what's used in Crystal's official Docker image (Older Ubuntu). Still, I tried updating, but it did not "just work": building bdwgc on Windows has been reworked.
    • Seems like that ability is a huge jump in difficulty, i.e. requires writing JavaScript code.
    • The separate script already exists and is linked from the wiki, but I found that GitHub Actions config was much more concise and expressive. Generally, grouping actions to a script makes the UI of builds less granular, though it's not so important here.

@RX14
Copy link
Contributor

RX14 commented Jan 12, 2020

building bdwgc on Windows has been reworked

It's actually been fixed: ntwin32.mak is no longer required. I'd prefer to move to the cmake build for bdwgc if possible.

@oprypin
Copy link
Member Author

oprypin commented Jan 12, 2020

It's possible to merge this and then update that.

@oprypin
Copy link
Member Author

oprypin commented Jan 12, 2020

Actually I'm convinced by your argument that it's much simpler now.
Willing to try, but for now getting linking errors, maybe you can advise.
https://github.com/oprypin/crystal/runs/385894088#step:8:2
oprypin@585a505

@RX14
Copy link
Contributor

RX14 commented Jan 13, 2020

@oprypin ah, it's fixed on latest bdwgc but not the release: https://github.com/ivmai/bdwgc/blob/7dcf6be3a74335349aab5bd38fc96594761e5780/CMakeLists.txt#L299.

You could probably just append that line to the cmakelists to get it to work. But there's still the /MT vs /MD problem... This is fixed with later cmake but you'd still have to patch the file to enable the feature.

For now, nmake -f NT_MAKEFILE make_as_lib=1 nodebug=1 on 8.0.4 looks like it should work.

@oprypin
Copy link
Member Author

oprypin commented Jan 13, 2020

No, didn't work either.
https://github.com/oprypin/crystal/runs/387467545
I am not pursuing this further.
Other than the remaining tiny dependency PR, this is ready.

@oprypin
Copy link
Member Author

oprypin commented Jan 13, 2020

Well, it can be merged even without #8674, it'll just fail 1 spec initially :>
So fully ready, actually.

@RX14
Copy link
Contributor

RX14 commented Jan 13, 2020

@oprypin oh! forgot cflags=-DDONT_USE_USER32_DLL in the nmake as well. Sorry, but I'm sure that that will fix it.

@oprypin
Copy link
Member Author

oprypin commented Jan 13, 2020

@RX14
Copy link
Contributor

RX14 commented Jan 13, 2020

ffs, we'll leave it as is then

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

1 small change and we are GTG.

I'm a inclined to allow failures on windows ci, but let's see how it how with the upcoming PRs.

.github/workflows/win.yml Outdated Show resolved Hide resolved
@oprypin

This comment has been minimized.

@oprypin
Copy link
Member Author

oprypin commented Jan 29, 2020

Ah no nevermind, it's just the old state of the repository because I'm not rebasing my branch.

Good to go.

But I have dropped the commit that allows Actions to also run on pull requests, to be on the safe side for now.

@straight-shoota
Copy link
Member

But I have dropped the commit that allows Actions to also run on pull requests, to be on the safe side for now.

I'd actually prefer running it on pull requests to immediately see whether the changes would break on windows. It's acceptable if builds occasionally break. That's the same with preview_mt.

Co-Authored-By: Brian J. Cardiff <bcardiff@gmail.com>
@bcardiff bcardiff merged commit 47bdbaa into crystal-lang:master Jan 29, 2020
@bcardiff
Copy link
Member

Let's see how it goes :-) Maybe if the free tier is not enough we might need to limit branch names.

Thanks @oprypin and others :-)

@oprypin
Copy link
Member Author

oprypin commented Jan 29, 2020

https://github.com/crystal-lang/crystal/runs/415569434 🎉

@j8r
Copy link
Contributor

j8r commented Jan 29, 2020

We could do the same for Alpine aarch64 🙏

@bcardiff
Copy link
Member

@j8r You mean #7420?

@straight-shoota
Copy link
Member

aarch64

@straight-shoota
Copy link
Member

@oprypin Didn't you say the actions wouldn't be triggered on pull requests? Am I missing something here: https://github.com/crystal-lang/crystal/pull/8683/checks?check_run_id=415666174
There's on: [push, pull_request] in the config file.

@oprypin
Copy link
Member Author

oprypin commented Jan 29, 2020

Oh sorry, I re-added that, per request

@bcardiff bcardiff added this to the 0.33.0 milestone Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continuous testing on Windows
5 participants