Skip to content

dev(ci): complete refactor #2966

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 8, 2021
Merged

dev(ci): complete refactor #2966

merged 5 commits into from
Apr 8, 2021

Conversation

oxy
Copy link

@oxy oxy commented Mar 25, 2021

Slowly reorganize CI steps and switch to using GH actions instead of docker images where possible. Initial test only replaces the fmt runner.

@oxy oxy force-pushed the rewrite-ci branch 9 times, most recently from c080bf1 to 022a339 Compare March 26, 2021 13:48
oxy
oxy commented Mar 26, 2021
View reviewed changes
@oxy oxy self-assigned this Mar 26, 2021
@oxy oxy force-pushed the rewrite-ci branch 2 times, most recently from 768ab3c to 29e39d8 Compare March 29, 2021 19:09
jsjoeio
jsjoeio suggested changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Great work! Initial pass looks 🔥

A couple things:

@oxy
Copy link
Author

oxy commented Mar 30, 2021

  • thank you for adding comments!

😄

  • nit: can we use spell Node with a capital "N"? 😛

Sure! patching that soon.

I'm hoping that since the CI is much faster in general (lint/fmt now only takes 3min), and since we cache wherever possible right now (currently yarn build dependencies, VSCode build - will refactor all the scripts in ci/ in a future PR once this lands, and also cache the package steps), we shouldn't strictly need to split it into multiple workflows. In addition, I'm not sure if its possible for items in one workflow to depend on another.

@jsjoeio
Copy link
Contributor

jsjoeio commented Mar 30, 2021

I'm hoping that since the CI is much faster in general (lint/fmt now only takes 3min), and since we cache wherever possible right now (currently yarn build dependencies, VSCode build - will refactor all the scripts in ci/ in a future PR once this lands, and also cache the package steps), we shouldn't strictly need to split it into multiple workflows. In addition, I'm not sure if its possible for items in one workflow to depend on another.

Ah, I see what you mean. Yeah not sure if that's possible either. Okay no worries then!

@jsjoeio jsjoeio mentioned this pull request Mar 30, 2021
Closed
@oxy oxy marked this pull request as ready for review March 31, 2021 17:04
@oxy oxy requested a review from a team as a code owner March 31, 2021 17:04
@oxy oxy requested a review from jsjoeio March 31, 2021 17:05
code-asher
code-asher previously requested changes Mar 31, 2021
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

I haven't been able to do a full review yet, just dropping this here so we remember to get the libc dependencies back down.

jsjoeio
jsjoeio reviewed Mar 31, 2021
View reviewed changes
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Nice work 🔥

Left one comment asking you to document a couple lines in the CI file. I'll let @code-asher give the final approval

jsjoeio
jsjoeio approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Wait for second approval but LGTM!

@jsjoeio jsjoeio changed the title Draft: Rewrite CI dev(ci): complete refactor Apr 1, 2021
@jsjoeio jsjoeio added this to the v3.9.3 milestone Apr 1, 2021
@oxy
Copy link
Author

oxy commented Apr 7, 2021

Hi there! This has been stalling for a while, mostly because building against an old version of glibc for CentOS 7 on a newer version of Ubuntu turns out to be not an easy process.

I haven't pushed these experiments, but I know they don't work:

  • glibc_version_header - repo says it doesn't work with libstdc++ easily
  • -static-libgcc -static-libstdc++ -static - does generate .o files instead of .so files but then node-gyp creates a shared object after the fact anyway :/
  • statifer on the .node led to weird bugs on my setup
  • I also looked at the node-gyp source code to see if I can force static .node files, but I felt lost in there so I bailed after a few hours.

Over the next days, I'll be looking at docker buildx, and at "dropping" compatibility for CentOS 7 for per-pull request CI and moving the slower CentOS 7 build to a tag/draft release workflow.

I really hope we can get new CI soon - I've been fighting this for weeks x.x

@jsjoeio
Copy link
Contributor

jsjoeio commented Apr 7, 2021

No worries at all! Thanks for the update.

Over the next days, I'll be looking at docker buildx, and at "dropping" compatibility for CentOS 7 for per-pull request CI and moving the slower CentOS 7 build to a tag/draft release workflow.

Is it possible to break up this work? i.e. get this PR to a stable state, review, merge and then do the remaining work in a follow-up PR? Just wondering if that would help at all.

@oxy
Copy link
Author

oxy commented Apr 7, 2021

We definitely could do that - the one thing I'm worried about is that we land this (and temporarily drop CentOS 7 support), and then we end up stalling on a follow-up that refactors our release workflow and gets CentOS 7 working again, and having to delay a release/manually build+upload binaries - given our v3.9.2 incident, I don't really want to repeat that >.<

@jsjoeio
Copy link
Contributor

jsjoeio commented Apr 7, 2021

given our v3.9.2 incident, I don't really want to repeat that >.<

Me neither! Sounds good to me then holding off.

@oxy oxy force-pushed the rewrite-ci branch 7 times, most recently from 0ba77fe to 712fe9d Compare April 8, 2021 14:38
@oxy oxy requested a review from code-asher April 8, 2021 16:54
@jsjoeio
Copy link
Contributor

jsjoeio commented Apr 8, 2021

@oxy did you address this?

just dropping this here so we remember to get the libc dependencies back down.

@jsjoeio jsjoeio dismissed code-asher’s stale review April 8, 2021 17:08

Because I am approving it

Akash Satheesan added 5 commits April 8, 2021 23:19
chore(ci): rewrite CI workflow
4bd10f2 
Nearly completely replace the original GitHub actions workflow.
Changes:

- Move from `.sh` files in `ci/steps` to steps in the workflow.
- Move from using docker images for environment to manual setup.
- Upgrade nfpm to v2.3.1

BREAKING CHANGE: official arm64 builds no longer support CentOS 7.
If you need to use CentOS 7 on arm64, build `code-server` locally.
For docs, see the yarn/npm section in `docs/install.md`.
chore(ci): adjust linux-amd64 package step
47218a9
chore(ci): add rsync for linux-amd64 package flow
394a7f1
chore(ci): add envsubst to centos:7 build flow
4860e50
chore(ci): re-add docker-arm64 workflow
e9e0b73
@oxy oxy merged commit 3a49299 into main Apr 8, 2021
@oxy oxy deleted the rewrite-ci branch April 8, 2021 18:12
@jsjoeio jsjoeio mentioned this pull request Apr 26, 2021
Closed
@jsjoeio jsjoeio added the chore Related to maintenance or clean up label May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsjoeio jsjoeio jsjoeio approved these changes

@code-asher code-asher Awaiting requested review from code-asher

Assignees

@oxy oxy

Labels
chore Related to maintenance or clean up
Projects
None yet
Milestone
v3.9.3
Development

Successfully merging this pull request may close these issues.
3 participants
@oxy @jsjoeio @code-asher