add github actions workflow for build and test on linux #973

toffaletti · 2020-04-01T15:48:21Z

Use the official swift docker images to execute make build test on pull requests targeting master. Creates a test matrix for swift and protobuf versions, currently running tests on both swift 5.1.5 and 5.2 with protobuf 3.11.4. The sub-targets of the test target are broken into steps to allow easier diagnostics and caching of the conformance test runner.

I tried to get caching of the conformance-test-runner working, but I haven't seen it work yet and I'm not sure why.

To see this working: /~https://github.com/toffaletti/swift-protobuf/pull/1/checks

- use matrix to parametrize swift and protobuf versions - cache conformance test runner

tbkka · 2020-04-01T18:26:39Z

Couple of notes/ideas:

We will need to be vigilant about updating the protobuf versions here -- in particular, the protocolbuffers project does regularly expand the conformance test and we do want to track that.

It might be worth asking the protocolbuffers project if they could include the conformance test binary in their release package (or a separate package?) to avoid the overhead of building it every time.

thomasvl · 2020-04-01T18:37:20Z

It might be worth asking the protocolbuffers project if they could include the conformance test binary in their release package (or a separate package?) to avoid the overhead of building it every time.

I sent some email to them directly to ask about it. But opening an issue in parallel on their project wouldn't hurt.

toffaletti · 2020-04-02T05:32:39Z

It might be worth asking the protocolbuffers project if they could include the conformance test binary in their release package (or a separate package?) to avoid the overhead of building it every time.

In theory the caching step should help with this, though I've yet to see it working. For example, I see messages in the logs that suggest it is caching something:

Post Cache conformance-test-runner

Running JavaScript Action with default external tool: node12
/bin/tar -cz -f /__w/_temp/4be5df96-f41a-4fac-a013-704eaa1092f9/cache.tgz -C /__w/swift-protobuf/swift-protobuf/protobuf/conformance .
Cache saved successfully

But in my limited testing the very next build still shows a cache miss:

Cache not found for input keys: Linux-pb-3.11.4-conformance-test-runner.

My suspicion is that it isn't working because I don't have this merged to master yet. The docs say there are some restrictions: https://help.github.com/en/actions/configuring-and-managing-workflows/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache

toffaletti · 2020-04-02T05:37:16Z

We will need to be vigilant about updating the protobuf versions here.

I might be able to partially automate this. Actions can be scheduled like a cronjob so one could potentially check for new protocol buffer releases and open a PR to add it to the test matrix automatically.

thomasvl · 2020-04-02T18:03:36Z

Actually, thinking some more about this, I think we might have a catch-22 here around the version of protocolbuffers/protobuf to use here.

All we need directly from that checkout is a built copy of protoc and conformance-test-runner. But not using head means we can't land support for things currently being worked on. i.e. - the conformance test won't validated newer check until the library is released (likely with fixes for other languages); if the new things are added to the .proto grammar, we can't have proto files in our repo ensuring they work because the older protoc we'd be using for test-plugin won't be able to parse them.

Should we just set up CI to always use master on protocolbuffer/protobuf? About the only other option I can see would be to try to teach the Makefile to query protoc for the version and then gate a bunch of our tests by the version aren't tested on CI until they make a protobuf release.

toffaletti · 2020-04-02T18:41:43Z

I think you could use branching to solve this. For example, master branch of swift-protobuf can be configured to pull and build from master branch of protocolbuffer. When you go to release a version of swift-protobuf you can make a branch and modify the CI to use latest version tag of protocolbuffer.

The problem I see with this approach is that breakage in protocolbuffer master becomes breakage in swift-protobuf master.

Another approach would be to have a dev or unstable branch of swift-protobuf that is pointed at protocolbuffer master. It could be used as a staging area for “breaking” changes such as adopting new unreleased protocolbuffer api or a new version of Swift and to ensure changes in protocolbuffer master are not breaking swift-protobuf. This would allow master swift-protobuf to remain stable and pinned against a released version. Presumably you’d not merge changes to master until protocolbuffer has actually released a version with those changes.

thomasvl · 2020-04-02T19:13:18Z

In the case of conformance fixes, we've land them immediately because it means the library is doing something wrong. And we usually do our own release shortly after to get all consumers on the correct behaviors. There's not to get SwiftProtobuf on the correct behavior asap and ensure it stays on the correct behavior.

New features in the proto grammar is more of a grey area. There really haven't been any that impacted SwiftProtobuf since we hit the 1.0 milestone, but that's not always going to remain true. If/when something new is added we'll need to be able to work on updating to support it. I'm not sure I follow the comment about master remaining "stable", "stable" would be releases and master should be evolving to be ready for the next release, no? If master were to stay pinned to an old version of protobuf it means we always have to scrabble when something outside our control does a release. If we force that work to happen on a branch, then it means we're potentially updating one (or more branches) with any other bug fixes/new features that also happen during that time span and/or still having a scramble to merge/resolve a branch against master on short order after they release.

Another option might not be to completely float on master, but to target a specific hash on master, that way if/when things like new conformance tests and/or new proto features are in flight, master's CI can be moved along with the matching changes here.

tbkka · 2020-04-02T19:31:54Z

... a dev or unstable branch of swift-protobuf ...

SwiftProtobuf's master branch is our dev branch. We do all development in master and branch for releases. (I know some people do it the other way around.)

The problem I see with this approach is that breakage in protocolbuffer master becomes breakage in swift-protobuf master.

Practically speaking, that hasn't been a problem. The PB folks do a pretty good job of keeping their master branch in usable shape.

New features in the proto grammar is more of a grey area.

But these happen so rarely that I think we'll do fine dealing with them as/when they occur. I don't see any need to modify anything here to specifically deal with such eventuality.

thomasvl · 2020-04-03T14:53:29Z

Just checked the conformance test, the master version has more tests at the moment. So by pinning to release we already start running a subset of what we likely should be always tracking.

New features in the proto grammar is more of a grey area.

But these happen so rarely that I think we'll do fine dealing with them as/when they occur. I don't see any need to modify anything here to specifically deal with such eventuality.

I guess my concern is are we saying the moment it happens we move to master? If so, why not just start there?

Anyway, I'll defer to @tbkka for where to start things out, but given some of the open upstream issues, I'm pretty sure we're we see changes soon.

thomasvl · 2020-04-03T14:54:48Z

Back to the main point of this CL -

Do we want to test back to our current min Swift toolchain? i.e. - 5.2 just GMed, so we officially support building with 4.2 still.
Can we do the same sorta setup for macOS?

thomasvl · 2020-04-03T14:55:52Z

.github/workflows/linux_build.yml

+    runs-on: ubuntu-18.04
+    strategy:
+      matrix:
+        swift: ["5.2", "5.1.5"]


Does "5.2" float? i.e. - is it 5.2 or does it auto pick up 5.2.1 since they just released that for linux? (and if/when there is a 5.2.2 would it get that)

It does not float, it will need to be updated. 5.2.1 had been released, but there were no docker images yet when I made this PR. As I said above, I think we can have a scheduled action for scraping for updates.

toffaletti · 2020-04-03T18:30:43Z

Back to the main point of this CL -

Do we want to test back to our current min Swift toolchain? i.e. - 5.2 just GMed, so we officially support building with 4.2 still.

I can add swift 4.2 to the matrix. Before I do this work, I wanted to resolve the issue of what version of protoc we want to target. If master, I need to make changes to the steps to build it.

toffaletti · 2020-04-03T18:32:46Z

Another option might not be to completely float on master, but to target a specific hash on master, that way if/when things like new conformance tests and/or new proto features are in flight, master's CI can be moved along with the matching changes here.

Sure, the line ref: v${{ matrix.protobuf }} for the protocol buffer checkout can be any git ref, I just need to make some changes to allow it to be more generic.

- allow using any gitref from protobuf repo - cache entire protobuf directory

toffaletti · 2020-04-04T21:29:41Z

OK, updates pushed. Highlights:

Swift 4.2.4, 5.2.1
protobuf current master commit hash added to matrix
Ability to specify any ref for protocolbuffers/protobuf in matrix
Added swift base image platform to matrix include and use in cache key otherwise the ubuntu-18.04 cache would fail on ubuntu-16.04. See this run for example: /~https://github.com/toffaletti/swift-protobuf/runs/560905478
protoc and conformance-test-runner built from git checkout used for all tests (no longer downloading pre-build protoc)
caching works, see this run for example: /~https://github.com/toffaletti/swift-protobuf/runs/560889219

As before, see toffaletti#1 for the actual runs of the workflow in the commits.

toffaletti · 2020-04-23T16:36:07Z

@tbkka @thomasvl friendly ping on this.

Another minor update, I got a workaround for building a more accurate cache key for protobuf: actions/checkout#209

thomasvl · 2020-04-23T16:51:45Z

So we just update that one hash when we want to test a newer version of master? Works for me.

thomasvl

Not that I follow most of the yml, but seems to make sense. 😄

thomasvl · 2020-04-23T16:54:06Z

ps - I really hope we can add macOS in a follow up.

tbkka · 2020-04-23T17:13:02Z

Merged. Let's see how it works. We can continue revising as needed.

thomasvl · 2020-04-23T17:18:29Z

Does something need to be enabled on the project? Or where does one look to see the post submit build/results for master?

Edit: /~https://github.com/apple/swift-protobuf/actions doesn't seem to list anything (and Actions isn't listed as a "tab" for this repo in general.

toffaletti · 2020-04-23T17:26:44Z

Not that I follow most of the yml, but seems to make sense. 😄

Feel free to ask any specifics, this was a learning project for me.

Does something need to be enabled on the project? Or where does one look to see the post submit build/results for master?

Maybe here in Settings?

thomasvl · 2020-04-23T17:29:01Z

@tbkka Looks like you will need to enable the actions then.

tbkka · 2020-04-23T17:35:36Z

I'll look into it. There may be some administrative hurdles to jump through, though.

toffaletti · 2020-04-25T18:01:08Z

.github/workflows/build.yml

+      matrix:
+        swift: ["5.2.1-bionic", "5.1.5-bionic", "4.2.4"]
+        # master as of 4/4/2020: cf601047ebf87cf7f443753ded41132eb689cb10
+        protobuf_gitref: ["v3.11.4", "cf601047ebf87cf7f443753ded41132eb689cb10"]


So we just update that one hash when we want to test a newer version of master? Works for me.

Yes, though this was a temporary workaround until I got an answer here: actions/checkout#209
With that I will be able to put "master" in this list and make the caching safe.

Is it worth landing that fix now while @tbkka works on getting the hook set up?

Yup, I'll work on it.

Issue 993 is a newer failure on updated conformance tests, so it would be nice to get our master validation update to float with those commits.

@toffaletti how do we check if the caching of things is working? Looking at /~https://github.com/apple/swift-protobuf/pull/994/checks it seems like it is building protobuf, I would have thought it would be cached from when @tbkka set things up and did a test PR to ensure they built?

I’ll take a look.

I agree it seems like the cache should have had a cache hit, in
/~https://github.com/apple/swift-protobuf/runs/664761605?check_suite_focus=true I see

Cache not found for input keys: Linux-ubuntu-18.04-protobuf-cf601047ebf87cf7f443753ded41132eb689cb10.

then later in Post Cache:

Cache saved successfully

And in the other parallel builds:

Running JavaScript Action with default external tool: node12
[warning]Cache already exists. Scope: refs/pull/991/merge, Key: Linux-ubuntu-18.04-protobuf-cf601047ebf87cf7f443753ded41132eb689cb10, Version: (null)

But then 3 days later in your PR a cache miss for the same key:

/~https://github.com/apple/swift-protobuf/runs/674773193?check_suite_focus=true

Cache not found for input keys: Linux-ubuntu-18.04-protobuf-cf601047ebf87cf7f443753ded41132eb689cb10.

However, I do see at least one cache hit:

/~https://github.com/apple/swift-protobuf/runs/675011233?check_suite_focus=true

Cache Size: ~182 MB (190677067 B)
/bin/tar -xz -f /__w/_temp/90437206-2a9e-46a5-9320-7515799d57c5/cache.tgz -C /__w/swift-protobuf/swift-protobuf/protobuf
Cache restored from key: Linux-ubuntu-18.04-protobuf-6935eae45c99926a000ecbef0be20dfd3d159e71

It seems like the cache isn't reliable. I will open an issue on /~https://github.com/actions/cache and update here.

actions/cache#310 - will see if they can investigate this better than I'm able to. Meanwhile I'll work on the improved caching for floating on master.

add github actions workflow for build and test on linux

baac1e3

- use matrix to parametrize swift and protobuf versions - cache conformance test runner

toffaletti force-pushed the github-actions branch from ed2525a to baac1e3 Compare April 1, 2020 15:50

tbkka requested a review from thomasvl April 2, 2020 19:32

thomasvl reviewed Apr 3, 2020

View reviewed changes

toffaletti added 6 commits April 4, 2020 11:22

always build protobuf

e521fd3

- allow using any gitref from protobuf repo - cache entire protobuf directory

add git commit hash to cache key

a997c4b

add cache fallback restore-key temporarily

8662ac2

fixes for caching and checkout

b0e6e4e

add swift 4.2.4, 5.2.1, and protobuf current master commit hash

76e9a7e

include swift image platform in cache key

7433747

toffaletti requested a review from thomasvl April 4, 2020 21:20

thomasvl requested a review from tbkka April 23, 2020 16:52

thomasvl approved these changes Apr 23, 2020

View reviewed changes

tbkka merged commit 0618427 into apple:master Apr 23, 2020

thomasvl mentioned this pull request Apr 23, 2020

Set up some form of CI #38

Closed

toffaletti commented Apr 25, 2020

View reviewed changes

thomasvl mentioned this pull request May 14, 2020

Update github actions so upstream master builds aren't pinned #998

Closed

toffaletti mentioned this pull request May 14, 2020

cache misses on cache key saved by previous PR actions/cache#310

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add github actions workflow for build and test on linux #973

add github actions workflow for build and test on linux #973

toffaletti commented Apr 1, 2020 •

edited

Loading

tbkka commented Apr 1, 2020

thomasvl commented Apr 1, 2020

toffaletti commented Apr 2, 2020

toffaletti commented Apr 2, 2020

thomasvl commented Apr 2, 2020

toffaletti commented Apr 2, 2020

thomasvl commented Apr 2, 2020

tbkka commented Apr 2, 2020

thomasvl commented Apr 3, 2020

thomasvl commented Apr 3, 2020

thomasvl Apr 3, 2020

toffaletti Apr 3, 2020

toffaletti commented Apr 3, 2020

toffaletti commented Apr 3, 2020

toffaletti commented Apr 4, 2020 •

edited

Loading

toffaletti commented Apr 23, 2020

thomasvl commented Apr 23, 2020

thomasvl left a comment

thomasvl commented Apr 23, 2020

tbkka commented Apr 23, 2020

thomasvl commented Apr 23, 2020 •

edited

Loading

toffaletti commented Apr 23, 2020

thomasvl commented Apr 23, 2020

tbkka commented Apr 23, 2020

toffaletti Apr 25, 2020

thomasvl Apr 27, 2020

toffaletti Apr 27, 2020

thomasvl May 14, 2020

thomasvl May 14, 2020

toffaletti May 14, 2020

toffaletti May 14, 2020

toffaletti May 14, 2020

add github actions workflow for build and test on linux #973

add github actions workflow for build and test on linux #973

Conversation

toffaletti commented Apr 1, 2020 • edited Loading

tbkka commented Apr 1, 2020

thomasvl commented Apr 1, 2020

toffaletti commented Apr 2, 2020

toffaletti commented Apr 2, 2020

thomasvl commented Apr 2, 2020

toffaletti commented Apr 2, 2020

thomasvl commented Apr 2, 2020

tbkka commented Apr 2, 2020

thomasvl commented Apr 3, 2020

thomasvl commented Apr 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toffaletti commented Apr 3, 2020

toffaletti commented Apr 3, 2020

toffaletti commented Apr 4, 2020 • edited Loading

toffaletti commented Apr 23, 2020

thomasvl commented Apr 23, 2020

thomasvl left a comment

Choose a reason for hiding this comment

thomasvl commented Apr 23, 2020

tbkka commented Apr 23, 2020

thomasvl commented Apr 23, 2020 • edited Loading

toffaletti commented Apr 23, 2020

thomasvl commented Apr 23, 2020

tbkka commented Apr 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toffaletti commented Apr 1, 2020 •

edited

Loading

toffaletti commented Apr 4, 2020 •

edited

Loading

thomasvl commented Apr 23, 2020 •

edited

Loading