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.

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

build: fail early in test-macos.yml #41035

Closed
wants to merge 1 commit into from
Closed

build: fail early in test-macos.yml #41035

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 30, 2021

No description provided.

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Nov 30, 2021
@targos
Copy link
Member

targos commented Nov 30, 2021

Why does npm ci fail?

@Trott
Copy link
Member Author

Trott commented Nov 30, 2021 •
edited
Loading

Why does npm ci fail?

It is reporting corrupted tar files from the npm registry.

Here are some links to some failures:

Here's what a failure looks like:

image

Possibilities I've considered but haven't been able to reproduce:

  1. Perhaps using -j2 in the workflow or something else is causing a race condition where two processes are running npm at the same time and trying to write the same tar files at about the same time, resulting in corrupted tar files.
  2. Perhaps our workflows in particular or GitHub's macOS instances in general are hitting the registry hard enough that the IP or IP range is being rate-limited, resulting in failed tar downlodas.
  3. Perhaps there is some bug that only occurs on macOS and/or only when npm is invoked the way it is in our tools/doc/node_modules task in the Makefile.

@targos
Copy link
Member

targos commented Nov 30, 2021

I think it would be nice to get input from someone at @nodejs/npm

@Trott
Copy link
Member Author

Trott commented Nov 30, 2021

It occurs to me that npm ci removes node_modules unconditionally so the failure may still happen anyway after the Build step. (Or maybe not because the relevant modules will be cached somewhere that npm-cli.js will find them? I was going to run the workflow a dozen times or whatever anyway to make sure it worked, so we'll probably find out if someone doesn't figure out a better way to address this issue.)

Trott
Trott commented Nov 30, 2021
View reviewed changes
.github/workflows/test-macos.yml Outdated
# that is causing the failure and the failure doesn't happen anymore if
# we run `npm ci` here first, that's also useful information.)
- name: npm ci workaround
run: (cd tools/doc && npm ci)
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this would be better?

Suggested change
run: (cd tools/doc && npm ci)
run: make tools/doc/node_modules

Copy link
Member Author

Choose a reason for hiding this comment

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

This might address the concern in #41035 (comment)?

@Trott Trott force-pushed the macos-ci branch 5 times, most recently from d780625 to 4d99903 Compare December 3, 2021 12:53
@Trott
Copy link
Member Author

Trott commented Dec 3, 2021

Five successful GitHub Actions runs in a row for this PR:

image

Not exactly conclusive, as only 1 of the 5 runs before that failed with the npm/ZDATA error. But promising so far.

@Trott Trott force-pushed the macos-ci branch 6 times, most recently from c628380 to f68edc4 Compare December 3, 2021 17:55
@richardlau
Copy link
Member

Why does npm ci fail?

It is reporting corrupted tar files from the npm registry.

Here are some links to some failures:

* https://github.com/nodejs/node/runs/4364074731?check_suite_focus=true

* https://github.com/nodejs/node/runs/4360537144?check_suite_focus=true

Here's what a failure looks like:

image

Maybe the macOS runners have some sort of proxy in front of the registry and we're seeing npm/cli#3884? Possibly fix isaacs/minipass#28.

@Trott
Copy link
Member Author

Trott commented Dec 4, 2021

Five more successful runs in a row:

image

@Trott
build: fail early in test-macos.yml
781408f 
PR-URL: nodejs#41035
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@Trott
Copy link
Member Author

Trott commented Dec 4, 2021

Landed in 781408f

@Trott Trott closed this Dec 4, 2021
@Trott Trott deleted the macos-ci branch December 4, 2021 05:30
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
build: fail early in test-macos.yml
9540b60 
PR-URL: #41035
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
build: fail early in test-macos.yml
6327685 
PR-URL: #41035
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 14, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@Trott @danielleadams
build: fail early in test-macos.yml
501e529 
PR-URL: #41035
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@Trott @danielleadams
build: fail early in test-macos.yml
f224bf3 
PR-URL: #41035
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@Trott @Linkgoron
build: fail early in test-macos.yml
950355f 
PR-URL: nodejs#41035
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
build: fail early in test-macos.yml
f826c86 
PR-URL: #41035
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
@mmomtchev
Copy link
Contributor

mmomtchev commented Jul 2, 2022 •
edited
Loading

@Trott I am getting this failure when building a Debian package inside of a Launchpad container - and only inside the Launchpad container - so I guess macOS is not to blame

PS No, in fact, for me npm is failing because it cannot access the registry - it is still the same error code, sorry for the noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Trott @targos @richardlau @mmomtchev @jasnell @nodejs-github-bot