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

build: remove test-tarball action for windows + osx #34440

Closed
wants to merge 2 commits into from

Conversation

MylesBorins
Copy link
Contributor

This GitHub action takes quite a bit of time to build and is regularly
flaky. Removing this action to avoid having red actions CI runs.

We definitely need to have testing in place for this, potentially
something in CI or alternatively an "opt-in" way of running this
so we can do something on releases or specific PRs.

Refs: #34123

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jul 20, 2020
@MylesBorins
Copy link
Contributor Author

We may want to hold off on landing this until we have a new solution... or at least open a tracking issue after landing to ensure we don't lose that action item.

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Since we have history of breaking building from the source tarball I’m very reluctant to remove this entirely (I’m not fussed if we ultimately test here or the Jenkins CI). As I mentioned over in #34123 (comment) I believe that the Linux test from tarball has been consistently passing so I would prefer we removed macOS and Windows (on a side note, it looked like it was a single test that was causing the Windows flakes, #34184) rather than the entire workflow.

This GitHub action takes quite a bit of time to build and is regularly
flaky. Removing the OSX and Windows target from this action to avoid
having red actions CI runs.

Refs: nodejs#34123
@MylesBorins MylesBorins changed the title build: remove build-tarball action build: remove test-tarball action for windows + osx Jul 22, 2020
@MylesBorins
Copy link
Contributor Author

I've updated the PR to keep the build-tarball and test-tarball-linux actions.

@mmarchini
Copy link
Contributor

mmarchini commented Jul 22, 2020

Recent (22 hours ago) build run where it fails without any logs: https://github.com/nodejs/node/runs/895604185?check_suite_focus=true. This error has been happening for months.

Note how the step that failed doesn't show any logs:

image

The problem is not Windows/OS X, there's something wrong with this action that generates undebuggable errors, and unless we come up with a way to debug this, I think we should remove the action entirely.

@richardlau
Copy link
Member

My suggestion would be to try changing the build tarball step to run on Linux instead of macOS.

@mmarchini
Copy link
Contributor

Ohhhh, I didn't realize it was running on OS X. Agreed, we should switch it to run on Linux.

@MylesBorins
Copy link
Contributor Author

In release-ci we use OSX to build the tarball... we could change this, but I do think it creates a risk of no longer testing the appropriate pipeline

@@ -13,7 +13,7 @@ jobs:
build-tarball:
env:
PYTHON_VERSION: 3.8
runs-on: macos-latest
runs-on: ubuntu-latest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this based on feedback in the PR review.

As we use OSX for building the tarball in production we might want to have a nightly job in travis to ensure that the pipeline we will be using in production is tested in some capacity.

@richardlau
Copy link
Member

In release-ci we use OSX to build the tarball... we could change this, but I do think it creates a risk of no longer testing the appropriate pipeline

We switched to building the source tarball on centos some time ago (nodejs/build#1777) for Node.js 12 and later. We switched Node.js 10 over in the release that went out this week as we need to migrate off the old macs by the end of the month (nodejs/build#2315).

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Contributor Author

Landed in 090ad7d

MylesBorins added a commit that referenced this pull request Jul 23, 2020
This GitHub action takes quite a bit of time to build and is regularly
flaky. Removing the OSX and Windows target from this action to avoid
having red actions CI runs.

Refs: #34123

PR-URL: #34440
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins added a commit that referenced this pull request Jul 27, 2020
This GitHub action takes quite a bit of time to build and is regularly
flaky. Removing the OSX and Windows target from this action to avoid
having red actions CI runs.

Refs: #34123

PR-URL: #34440
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@ruyadorno ruyadorno mentioned this pull request Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants