GitHub Actions by waj · Pull Request #9514 · crystal-lang/crystal

waj · 2020-06-19T22:47:51Z

This is the first step to move CI of PRs and merges to GitHub Actions so we can leave CircleCI for releases. Still lacking some features like saving some artifacts, nicer error reporting or publishing docs from master branch.

I had to make some changes to make this work:

On macOS openssl must be reinstalled because the environment already have an older version and then it was renamed by Homebrew. So it detects it as installed but it's not.
The log output in GitHub Actions doesn't update until each line is flushed, and this is really annoying while the tests are running. That's why I added the option to specify line splits with an environment variable. Setting SPEC_SPLIT_DOTS with an integer value will print a newline every that number of specs are executed. Probably the most controversial change of this PR :)
Also to improve the readability on GitHub, surround each command executed by bin/ci with special grouping lines so they show as foldable.

jhass · 2020-06-19T23:01:58Z

Cool :) Maybe let's just drop Travis at the same time and refactor bin/ci to not carry it around anymore?

jhass · 2020-06-19T23:06:42Z

.github/workflows/linux.yml

+        run: bin/ci prepare_build
+
+      - name: Test
+        run: bin/ci with_build_env 'make std_spec threads=1'


What do you think of trying to run the compiler specs here too?

The compiler specs are executed later using the compiled compiler. But it takes a lot of time, so this is just a "fail fast" step to catch more common issues before that.

Note I'm talking about test_linux_32 specifically here. I don't see any other 32bit tests here?

Oh, sorry! Yup, that's probably a good idea. I just transferred as literal as possible what's currently running on CircleCI, but I don't know why it's not running the whole suite with bin/ci build. @bcardiff do you know why? Resource limits maybe?

The reason was that the resources were not enough to build the compiler in 32 bits environment. I doubt they will be able to succeed.

Ref: #8283

I mean otoh, while things like 32bit ARM is still widely used, 32bit x86 is basically dead anyways. I guess we wouldn't be too terrible people if we just drop it as a platform when most Linux distributions and OSes already did so.

When we have some other 32 bits platform, I will accept to drop 32bits x86 builds. If we drop it before I fear there will be more things broken on 32 bits due to 64 bit assumptions.

Maybe what we can do is to drop the 32 bits compiler and use cross-compilation to test 32 bits std-spec. If we do things that way it will be less things to maintain in the release process. And at the same time pave the way for something between tier 1 and tier 2 (or improve what is offered in tier 2).

I pocked a bit at 32bit ARVMv7 support in the last couple of days and the current state is... not pretty.

Stdlib specs break on some random OpenSSL session handshake rejects that sound more like memory corruption than anything else, or perhaps another round of C ABI issues.

Atomic requires some more from compiler-rt on the platform and just linking with clang -rtlib=compiler-rt had me run into all kinds of linker issues.

compiler_spec just runs out of memory no matter what I do, that is not actual memory but what it can allocate via bdwgc, regardless of --enable-large-config.

Running an armhf docker container on an aarch64 host was so broken, not even date could do anything but display random values.

My attempt to virt-install --arch=armv7l on the aarch64 host would just have the VM hog a CPU and not even show a POST message via virsh console.

The icing on the cake is that non of my test setups would then even run into the LFS issues other people are seeing. So just setting up a CI somehow doesn't mean we're actually testing that part apparently...

So, that's some road ahead of us and will likely be in the Windows kind of state for a while where we just can run a whatever works right now CI.

bumpy road ahead 😞 .

Let's decouple the discussion of better 32 bits infra for after this PR then. Do you agree @jhass?

Sounds good.

waj · 2020-06-19T23:11:58Z

drop Travis at the same time and refactor bin/ci to not carry it around anymore?

Sure, but let's first make sure this works fine and cover our needs. Travis build is kind of abandoned anyway and seems like it's only there to make noise on our Slack channel 😄

oprypin · 2020-06-20T07:45:20Z

Umm just so this isn't missed, looks like there are some errors being printed and skipped?

https://github.com/crystal-lang/crystal/runs/789681861?check_suite_focus=true#step:5:2433

Might want to try printing the ::group stuff to stderr

straight-shoota · 2020-06-20T12:47:52Z

Probably the most controversial change of this PR :)

This immediately caught my eye. It feels just weird. Did you do any digging into why Github Actions doesn't flush immediately and wheter it may be possible to change that?

If not, I'd still question whether we really need this. I mean it's only really useful if you're monitoring the CI output in real-time. Is that really that common? I mean it takes a long time to run completely, anyways. So is there any real benefit for having a quicker updated output? And if it is important, it begs the question if we should really switch to a CI tool that doesn't support this when the current one works perfectly for this.

Alternatively, we could consider an option to split output lines by internal groupings, for example per spec file. That may be more useful than arbitrary amounts? But then it's also less predictable. And I guess using 160 chars nicely lines up with 2 lines of 80 chars.

asterite · 2020-06-20T13:19:15Z

Someone should run a ruby project with rspec doing sleep 1 in each test, and seeing if that works fine. If so, then I'm sure we don't need that change. But I also agree that real time output for specs is not important in CI.

j8r · 2020-06-20T14:13:14Z

What is the reason motivating this PR? As far as I have read CircleCI will still be used (for releases).

waj · 2020-06-20T14:51:57Z

Most of the time we don't need to monitor CI in real time, but if you go to a running workflow and try to expand the current log, it doesn't show up until a new line is written. Also, really long lines made the entire log to break and not show at all after that line, even after the job finishes. I know these are probably issues that GitHub will eventually fix. I don't want to add more features to specs. That is just a very simple workaround that we could leave undocumented and remove as soon as we don't need it anymore.

Motivation behind this PR: jobs running in CircleCI are many times lagging behind. Specially macOS jobs. @bcardiff often has to cancel jobs in order to run release tasks. GitHub allows higher parallelism, even in the free plan (https://help.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits). Also, @jhass is working on using an external runner for ARM. Once it's done we would have all the environments running in a single place.

CircleCI is still superior option in some other aspects, specially maturity. That's why I don't pretend to replace in a single move our CI environment. I just want to run this side by side and evaluate how it behaves for us. But it's impossible to predict without real usage.

jhass · 2020-06-20T14:59:23Z

Also, @jhass is working on using an external runner for ARM. Once it's done we would have all the environments running in a single place.

FWIW it totally is done, just pending review and merge. #9508 is draft because none of the PRs it depends on got any attention.

waj · 2020-06-20T15:18:53Z

Umm just so this isn't missed, looks like there are some errors being printed and skipped?

@oprypin that was me poorly using shell interpolations :). Should be fixed now.

asterite · 2020-06-20T16:29:29Z

What if we change spec to always output a newline after 80 specs? That is, without a configuration and it will also show nicely in consoles (plus you can kind of count how many tests already ran). Maybe make it 100 or 50 to make it even easier to count

jhass · 2020-06-20T16:34:54Z

That seems like a neat solution until somebody ports fuubar :D

straight-shoota · 2020-06-20T16:59:45Z

A better solution than altering spec implementation would be to insert newlines to the command output externally. For example piping through fold should work for that: make compiler_spec | fold -w80 -
This would keep the workaround contained to the environment where it's necessary.

Blacksmoke16 · 2020-06-20T17:47:36Z

.github/workflows/linux.yml

@@ -0,0 +1,101 @@
+name: Linux CI
+
+on: [push, pull_request]


Won't this produce duplicate builds? I.e. one for the push and another for the PR?

Well it does produce duplicates in some sense, when core contributors push a branch to the main repo and then create a PR from that. It's possible to tell it to test only when the push is to master, but then you lose the ability to just push a branch just for the purpose of running CI on it.

So this is fine. It's been this way in win.yml since the start, anyway.

waj · 2020-06-20T18:13:02Z

Oh, I didn’t know about fold. That seems to be the right answer! :)

waj · 2020-06-20T22:15:23Z

I just rolled back the changes in spec and use fold instead. But does anyone have any idea why is splitting in lines every 16 chars instead of 160? 😆

oprypin · 2020-06-20T22:36:59Z

Probably because each dot is �[32m.�[0m

waj · 2020-06-20T23:10:52Z

Yup, I was about to comment the same. The command is executed within a docker container with --tty so it doesn't detect that the output is not actually the tty. It cannot be used on Windows either, so... I don't know. I just reverted the last changes. I still think that a really simple workaround within spec itself doesn't hurt at all. It doesn't have any effect unless the SPEC_SPLIT_DOTS variable is set.

asterite · 2020-06-21T01:25:59Z

You can pass --no-color to spex

oprypin · 2020-06-21T06:03:02Z

Ok but colors are cool too

straight-shoota · 2020-06-21T12:11:01Z

I guess multiplying the width by 10 would yield the expected result then? make compiler_spec | fold -w800 -
For windows you could use something like this: bin/spec | %{$_ -replace ".{80}", "$&\n"}

bcardiff

I think the current status is good enough for stressing the usage of GitHub Actions for CI on PRs.

waj added 6 commits June 19, 2020 19:29

GitHub Actions for Linux build

72bead3

Added macOS workflow

fc9745c

Break spec output lines

7b84b25

Group output on GitHub Actions

15e2e4d

Alpine, MT and 32bit jobs for Linux CI

9183425

Check format job

78d9c5a

waj added the topic:infrastructure label Jun 19, 2020

jhass reviewed Jun 19, 2020

View reviewed changes

Fix interpolation for GitHub log grouping

062d81d

Blacksmoke16 reviewed Jun 20, 2020

View reviewed changes

waj force-pushed the github-actions branch from 1f0ead8 to 062d81d Compare June 20, 2020 23:09

bcardiff approved these changes Jun 22, 2020

View reviewed changes

asterite mentioned this pull request Jun 22, 2020

Regression in 0.35: Hash#each with block receiving tuple #9450

Closed

bcardiff added this to the 1.0.0 milestone Jun 22, 2020

bcardiff merged commit bab17be into crystal-lang:master Jun 22, 2020

waj deleted the github-actions branch July 13, 2020 15:15

Uh oh!

Conversation

waj commented Jun 19, 2020

Uh oh!

jhass commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhass Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waj commented Jun 19, 2020

Uh oh!

oprypin commented Jun 20, 2020

Uh oh!

straight-shoota commented Jun 20, 2020

Uh oh!

asterite commented Jun 20, 2020

Uh oh!

j8r commented Jun 20, 2020

Uh oh!

waj commented Jun 20, 2020

Uh oh!

jhass commented Jun 20, 2020

Uh oh!

waj commented Jun 20, 2020

Uh oh!

asterite commented Jun 20, 2020

Uh oh!

jhass commented Jun 20, 2020

Uh oh!

straight-shoota commented Jun 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waj commented Jun 20, 2020

Uh oh!

waj commented Jun 20, 2020

Uh oh!

oprypin commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

waj commented Jun 20, 2020

Uh oh!

asterite commented Jun 21, 2020

Uh oh!

oprypin commented Jun 21, 2020

Uh oh!

straight-shoota commented Jun 21, 2020

Uh oh!

bcardiff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jhass commented Jun 19, 2020 •

edited

Loading

jhass Jun 22, 2020 •

edited

Loading

oprypin commented Jun 20, 2020 •

edited

Loading