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

Update CI to publish binary releases on git version tag #27

Merged
merged 14 commits into from
May 23, 2020

Conversation

almarklein
Copy link
Collaborator

This adds jobs to the Github action CI workflow, that build binaries suited for distribution. On commits that are tagged with v..., a matching release is made and the binaries (packed into zipfiles) are uploaded as assets.

I basically took the Azure pipeline code from wgpu-bin, translated it to Github Actions, and weaved in into the existing CI. I deliberately made a new release-build job, so we can keep these builds separate from the test-build jobs.

I tested this on my own fork (Github Actions Just Work on forks!), I pushed a tag, and some minutes later the releases appeared.

So to publish a release, we must bump the version in cargo.toml, and then git tag v.. and git push that tag. I considered detecting the version bump and creating a tag automatically, but IMO that feels like too much automation. Open for discussion though.

We have quite a lot of jobs now, some kind of duplicate. We could consider removing (some of) the stable test builds.

@almarklein
Copy link
Collaborator Author

@kvark would you mind if I squash-merged this when we're ready? If I read it correctly, I can add co-authors then, and I'd like to attribute @Korijn, who authored wgpu-bin.

@almarklein
Copy link
Collaborator Author

almarklein commented May 19, 2020

Two of the new builds pull a Docker image from quay.io, (by RedHat) which has been having issues all day. (had to restart CI a few times just now to make it work) I assume that should normally be pretty stable :)

Ubuntu Nightly,
Windows Stable,
Windows Nightly
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this, as it's not needed, I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out this was here for bors, see bors.toml :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does it though? The jobs still have the same names. I don't think Bors checks this file or anything, it just listens for success/fail events with matching names, so I think it's fine. Unless I'm missing something.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you, this looks very promising (and sophisticated:) )

name: Publish release artifacts
runs-on: ubuntu-18.04
# Builds for binary releases.
# Also build on non-tag commits; it also functions as a test.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an idea of how much slower this would make our general CI? I.e. does using an image affect the build times?

@Korijn
Copy link
Contributor

Korijn commented May 20, 2020

In my experience, the rust build still largely dominates the time. The docker image is pretty small so its pulled in no time.

Running it within a docker image on Linux shouldn't effect the build time at all.. it's still running as a process on the host (no virtualization or anything)

@almarklein
Copy link
Collaborator Author

Looking at the reported times for each job, they are indeed very much comparable. The Windows builds take the longest, but they take about as long as the Windows builds that were already done.

There are more jobs now, so potentially it can take more time if there's a queue for the pool of runners.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Alright, thank you!

@kvark
Copy link
Member

kvark commented May 20, 2020

@grovesNL would you be interested to look at this as well? I think it would be useful. Otherwise, we'll proceed.

uses: actions/upload-artifact@v2
with:
path: commit-sha
name: dist
Copy link
Contributor

@Korijn Korijn May 20, 2020

Choose a reason for hiding this comment

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

Unlike wgpu-bin where we have to clone the source repo prior to building, here you can just create the commit-sha file in the final publish job and avoid the prepare job completely. It will allow queueing the subsequent jobs earlier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd assumed the build needed it, but make package already does that. We can thus omit this step completely, because the GH release page already reports the commit sha :)

IMAGE: ${{ matrix.IMAGE }}
RUST_TOOLCHAIN: ${{ matrix.RUST_TOOLCHAIN }}
run: |
CID=$(docker create -t -w /tmp/wgpu-native -v $PWD:/tmp/src:ro quay.io/pypa/$IMAGE bash -c "\
Copy link
Contributor

@Korijn Korijn May 20, 2020

Choose a reason for hiding this comment

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

Don't you need to preface all of the bash scripts with set -ex to make sure we don't steamroll errors? Or is that by default on github actions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default is set -e (-like behavior), which can be turned off with fail-fast: false

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that's fine then

Copy link
Collaborator

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

Thank you! This looks great to me.

The Docker idea is interesting and makes we wonder whether we should be doing something similar in gfx-portability too.

name: Publish release artifacts
runs-on: ubuntu-18.04
# Builds for binary releases.
# Also build on non-tag commits; it also functions as a test.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd slightly prefer to only bother building tagged commits to avoid the potential for CI backlog, but we could start with this and see how it goes

@kvark
Copy link
Member

kvark commented May 21, 2020

Sounds like we are good to merge this.
There are concerns voiced by @Korijn though that would need to be resolved.
bors delegate=Korjin

@bors
Copy link
Contributor

bors bot commented May 21, 2020

✌️ KORJin can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@grovesNL
Copy link
Collaborator

(username typo)
bors delegate=Korijn

@bors
Copy link
Contributor

bors bot commented May 21, 2020

🔒 Permission denied

Existing reviewers: click here to make grovesNL a reviewer

@grovesNL
Copy link
Collaborator

cc @kvark ^

@kvark
Copy link
Member

kvark commented May 21, 2020

bah
bors delegate=Korijn

@bors
Copy link
Contributor

bors bot commented May 21, 2020

✌️ Korijn can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@kvark
Copy link
Member

kvark commented May 21, 2020

@grovesNL sorry about that! fixed now, you should have full control here.

@Korijn
Copy link
Contributor

Korijn commented May 21, 2020

No worries, it happens literally all the time! :)

@Korijn
Copy link
Contributor

Korijn commented May 21, 2020

@grovesNL just to make sure we are all clear on why we're using that particular docker container:

  • it helps create highly portable binaries for linux for reasons better explained here: https://www.python.org/dev/peps/pep-0513/
  • the python package index only accepts binary packages that adhere to the linked PEP513 standard, so it was a requirement for wgpu-py

@Korijn
Copy link
Contributor

Korijn commented May 21, 2020

LGTM. @almarklein might want to give it one last full run at your fork?

@almarklein
Copy link
Collaborator Author

@Korijn That was a good call. Some fixes that I had earlier made to the release job had disappeared as I prepared this new branch. Anyway:

λ git tag v0.5.test5
λ git push origin v0.5.test5  # (origin being my fork)

-> https://github.com/almarklein/wgpu-native/releases/tag/v0.5.test5

@Korijn
Copy link
Contributor

Korijn commented May 21, 2020

bors r+

@kvark
Copy link
Member

kvark commented May 21, 2020

Strange, let me try
bors r=Korijn

@kvark
Copy link
Member

kvark commented May 21, 2020

It shows some internal server error :/
Do we need to update bors.toml?

@Korijn
Copy link
Contributor

Korijn commented May 21, 2020

@almarklein any idea why build status is still pending?

@kvark
Copy link
Member

kvark commented May 21, 2020

We'll need to merge this manually. That's ok. But let's also update "bors.toml" with the new task names.

@kvark
Copy link
Member

kvark commented May 22, 2020

Also somewhat minor - there is a separate section in Readme describing the binary releases, which we should update according to this PR.

@kvark
Copy link
Member

kvark commented May 22, 2020

Are we all set here?

@almarklein
Copy link
Collaborator Author

I think so. I made the release-builds only build for tags. The docker registry that the image is to be pulled from is having issues again, so I'm taking this as a sign that it's a liability.

Other than that, what up with that failing MacOS build suddenly?

@Korijn
Copy link
Contributor

Korijn commented May 23, 2020

I think so. I made the release-builds only build for tags. The docker registry that the image is to be pulled from is having issues again, so I'm taking this as a sign that it's a liability.

quay.io is a very very common docker image registry, it's not normal for it to have issues like this.

Other than that, what up with that failing MacOS build suddenly?

It fails when compiling dependency ron, probably an upstream issue?

   Compiling ron v0.5.1
thread 'rustc' panicked at 'index out of bounds: the len is 0 but the index is 0', src/librustc_mir_build/hair/pattern/_match.rs:1184:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.45.0-nightly (9310e3bd4 2020-05-21) running on x86_64-apple-darwin

note: compiler flags: -C embed-bitcode=no -C debuginfo=2 --crate-type lib

note: some of the compiler flags provided by cargo are hidden

error: could not compile `gfx-backend-metal`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
##[error]Process completed with exit code 101.

@kvark
Copy link
Member

kvark commented May 23, 2020

This is coming from gfx-backend-metal (and not RON), filed as rust-lang/rust#72467
This PR needs to be merged manually, so it's not a blocker for you.

@kvark kvark merged commit f22fa51 into gfx-rs:master May 23, 2020
@almarklein almarklein deleted the cd2 branch May 23, 2020 21:19
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.

4 participants