Skip to content

lookup: skip some permanently failing modules#880

Merged
targos merged 1 commit intonodejs:mainfrom
targos:skip-v17
Nov 29, 2021
Merged

lookup: skip some permanently failing modules#880
targos merged 1 commit intonodejs:mainfrom
targos:skip-v17

Conversation

@targos
Copy link
Member

@targos targos commented Nov 10, 2021

No description provided.

SimenB
SimenB reviewed Nov 10, 2021
View reviewed changes
voxpelli
voxpelli reviewed Nov 15, 2021
View reviewed changes
ljharb
ljharb reviewed Nov 15, 2021
View reviewed changes
mcollina
mcollina reviewed Nov 15, 2021
View reviewed changes
@voxpelli
Copy link

voxpelli commented Nov 18, 2021

I would propose that one or more issues gets created for these, linked to issues/PR:s in the respective project, to track this.

Then I could be 👍 to merge this as an emergency measure to eradicate the alert fatigue that is currently affecting citgm on Node >=17 .

Without a tracking/plan for how to reactivate these modules I don't like the idea really :/

@targos targos marked this pull request as draft November 18, 2021 15:19
mcollina
mcollina reviewed Nov 18, 2021
View reviewed changes
@targos
Copy link
Member Author

targos commented Nov 18, 2021

I'm going to make a release with the package download fix and rerun citgm to see where we are now. then i'll update this

@targos
Copy link
Member Author

targos commented Nov 18, 2021

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/1110/

@targos targos marked this pull request as ready for review November 18, 2021 19:29
@targos
Copy link
Member Author

targos commented Nov 18, 2021

Updated:

  • Unskipped pino and npm because they are being worked on.
  • Did not skip fastify because it's being fixed in Node.js
  • Unskipped Jest so we can handle it separately
  • Unskipped @hapi/shot as it seems fixed
  • Added useGitClone: true to cheerio because of the error: fatal: not a git repository (or any of the parent directories): .git
  • Skipped socket.io because its native dependency isn't compatible with Node.js 17

@codecov-commenter

This comment has been minimized.

Sign in to view
@dominykas
Copy link
Member

dominykas commented Nov 19, 2021

Without a tracking/plan for how to reactivate these modules I don't like the idea really :/

They could be skipped only until a certain date? i.e. reduce the noise for a couple of months, revisit later?

@targos
Copy link
Member Author

targos commented Nov 19, 2021

Without a tracking/plan for how to reactivate these modules I don't like the idea really :/

They could be skipped only until a certain date? i.e. reduce the noise for a couple of months, revisit later?

Of course. The goal is not to skip forever.

@dominykas
Copy link
Member

dominykas commented Nov 19, 2021 •
edited
Loading

Ah, it's a JSON... can't have a date check in the config [without extra new code]. Sorry, distraction.

richardlau
richardlau reviewed Nov 19, 2021
View reviewed changes
lib/lookup.json
"maintainers": "blakeembrey",
"skip": ["aix"]
"skip": ["aix", ">=17"],
"comment": "OpenSSL 3.0"
Copy link
Member

@richardlau richardlau Nov 19, 2021

Choose a reason for hiding this comment

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

FWIW the tests appear to pass with --openssl-legacy-provider, see https://ci.nodejs.org/job/citgm-smoker-nobuild/1111/ which uses NODE_OPTIONS which we could set via envVar.

Copy link
Member

@richardlau richardlau Nov 19, 2021

Choose a reason for hiding this comment

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

ah I guess that wouldn't work for older versions of Node.js 😞.

@targos
Copy link
Member Author

targos commented Nov 24, 2021

Are we good with the current patch?

@targos
Copy link
Member Author

targos commented Nov 29, 2021

@nodejs/citgm

mcollina
mcollina approved these changes Nov 29, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@targos targos merged commit 7f774e9 into nodejs:main Nov 29, 2021
@targos targos deleted the skip-v17 branch November 29, 2021 13:57
GeoffreyBooth
GeoffreyBooth reviewed Nov 29, 2021
View reviewed changes
lib/lookup.json
"coffeescript": {
"maintainers": ["jashkenas", "GeoffreyBooth"],
"skip": "rhel"
"skip": ["rhel", "aix"]
Copy link
Member

@GeoffreyBooth GeoffreyBooth Nov 29, 2021

Choose a reason for hiding this comment

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

I wasn’t aware of this . . . there’s not really any reason I can think of that CoffeeScript should fail in any environment.

Copy link
Member

@richardlau richardlau Nov 29, 2021

Choose a reason for hiding this comment

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

AIX: https://ci.nodejs.org/job/citgm-smoker/nodes=aix72-ppc64/2790/testReport/junit/(root)/citgm/coffeescript_v2_6_1/

 npm ERR! code 1
 npm ERR! path /home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/coffeescript/node_modules/puppeteer
 npm ERR! command failed
 npm ERR! command sh -c node install.js
 npm ERR! /home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/coffeescript/node_modules/puppeteer/lib/cjs/puppeteer/common/assert.js:26
 npm ERR!         throw new Error(message);
 npm ERR!               ^
 npm ERR! 
 npm ERR! Error: Unsupported platform: aix
 npm ERR!     at Object.assert (/home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/coffeescript/node_modules/puppeteer/lib/cjs/puppeteer/common/assert.js:26:15)
 npm ERR!     at BrowserFetcher.setPlatform (/home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/coffeescript/node_modules/puppeteer/lib/cjs/puppeteer/node/BrowserFetcher.js:180:25)
 npm ERR!     at new BrowserFetcher (/home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/coffeescript/node_modules/puppeteer/lib/cjs/puppeteer/node/BrowserFetcher.js:164:14)
 npm ERR!     at PuppeteerNode.createBrowserFetcher (/home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/coffeescript/node_modules/puppeteer/lib/cjs/puppeteer/node/Puppeteer.js:185:16)
 npm ERR!     at downloadBrowser (/home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/coffeescript/node_modules/puppeteer/lib/cjs/puppeteer/node/install.js:42:46)
 npm ERR!     at download (/home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/coffeescript/node_modules/puppeteer/install.js:86:3)
 npm ERR! 
 npm ERR! Node.js v18.0.0-pre
 npm ERR! A complete log of this run can be found in:
 npm ERR!     /home/iojs/tmp/citgm_tmp/28cfdfc7-146e-4343-9f50-352435316c6c/home/.npm/_logs/2021-11-06T19_04_46_623Z-debug.log

Copy link
Member

@GeoffreyBooth GeoffreyBooth Nov 29, 2021

Choose a reason for hiding this comment

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

Ah, the tests, that makes sense. If I wanted to get the tests running in AIX, say by disabling Puppeteer in AIX, is there some way I could launch an AIX environment to test that my changes work?

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

Reviewers

@ljharb ljharb ljharb left review comments

@richardlau richardlau richardlau left review comments

@mcollina mcollina mcollina approved these changes

+5 more reviewers

@voxpelli voxpelli voxpelli left review comments

@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments

@dominykas dominykas dominykas left review comments

@SimenB SimenB SimenB left review comments

@BethGriggs BethGriggs BethGriggs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@targos @voxpelli @codecov-commenter @dominykas @ljharb @mcollina @GeoffreyBooth @SimenB @richardlau @BethGriggs