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

Log failed optional platform dependencies as info #169

Conversation

cmedinasoriano
Copy link

The intention of this PR is to continue the discussion on npm/npm#19198
All credits to this fix goes to @jacksonrayhamilton
My only contribution was to apply the same fixes to current npm/cli and opening the discussion again.

This was the PR message for npm/npm#19198 (with corrected issue refs to npm/npm):
Fixes npm/npm#11632, reducing the log level of nonfatal errors when installing optional dependencies on an unsupported platform (e.g. fsevents on GNU/Linux). Adapts the changes from npm/npm#12841 to the current state of the repo and endeavors to implement the requested changes in this issue comment.

In the last two PRs that attempted to address this, it was suggested a "debug" error level be used, but it does not seem like "debug" exists as an error level in npmlog. "info" seems to have similar intent though, and it does eliminate the message by default:

~/projects/chokidar-test λ cat package.json 
{
  "dependencies": {
    "chokidar": "^1.7.0"
  }
}
~/projects/chokidar-test λ npm i
npm WARN chokidar-test No description
npm WARN chokidar-test No repository field.
npm WARN chokidar-test No license field.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.3 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.1.3: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

added 116 packages in 4.831s
~/projects/chokidar-test λ ../npm/bin/npm-cli.js i
npm WARN chokidar-test No description
npm WARN chokidar-test No repository field.
npm WARN chokidar-test No license field.

added 116 packages in 4.122s

@cmedinasoriano cmedinasoriano requested a review from a team as a code owner February 24, 2019 18:49
@cmedinasoriano cmedinasoriano force-pushed the log-failed-optional-platform-dependencies-as-info branch from a2a4017 to c48fc7c Compare February 24, 2019 20:00
@zkat zkat added semver:major backwards-incompatible breaking changes needs-discussion labels Mar 4, 2019
@13rac1
Copy link

13rac1 commented Mar 15, 2019

@zkat I suggest not labeling this needs-discussion. This issue has already been bikeshed for years. Originally starting four years ago in npm/npm#8921 with a detailed summary in npm/npm#11632 (comment)

@aeschright
Copy link
Contributor

We use needs-discussion to flag that the team may need to make an internal decision or coordinate with other parts of the organization. In this case, because the change is marked semver-major, the soonest it can be merged will be the npm 7 release in a few months.

@Allactaga
Copy link

@aeschright, I am not familiar with the internal processes of the team. Can you please share when the discussion will take place? Will you make sure that this change will go out with npm7 and we don't have to wait for npm8?

@herebebeasties
Copy link

@aeschright / @zkat: Why does omitting a log message like this count as public API surface area worthy of a major semver update?

Given that the community clearly would like it fixed (literally hundreds of comments/votes across multiple issues) and that the actual substantial change here is a totally trivial three line patch, that it's so far taken more than four years to look at this given all the activity and comments around it is frankly mind-boggling. It massively puts people off contributing to the project. I certainly wouldn't bother, looking at how all this has been handled. 👎

@jacksonrayhamilton
Copy link

@herebebeasties Exactly. You hit the nail on the head. Let NPM burn! 🔥 🔥

@cmedinasoriano, I implore you to close this PR. Let’s let NPM fail. Let’s tell our friends to switch to Yarn because NPM is a poorly-run project.

@jacksonrayhamilton
Copy link

NPM can either improve, or die. It may have been a mistake for Node to include this company’s CLI in the Node installation. The company can prove to us whether that’s the case or not. If we will sentiment to change, then a better package manager, or least better stewardship, could be available to us in the future.

@ljharb
Copy link
Collaborator

ljharb commented May 21, 2019

That’s a pretty strong reaction to a PR about a log message.

Speaking as a maintainer myself, the age of an issue does not have any correlation whatsoever to its priority, its risks, or the speed of addressing it.

@jacksonrayhamilton
Copy link

I dedicated several hours of my time to learn how the project worked, and in more than one instance provided code and tests, all free of charge. How many people pause their lives to do that?

The first time I did that, they missed my contribution and commented months later that it’d been fixed elsewhere. The second time, I advocated for my change for months, only to see the issue closed by a bot. Scroll through the hundreds of comments and reactions over the past years in relation to this “pitiful, insignificant” log message update to gauge why NPM is truly a poorly-run project.

And at the end of all of it, I get to read snarky comments like yours, implying that I should sit down and shut up. Nope, no more of that.

@ljharb
Copy link
Collaborator

ljharb commented May 21, 2019

I’m not being snarky; but while it’s commendable for you to donate your time and effort, that in no way obligates anyone else to do so, including npm.

@jacksonrayhamilton
Copy link

Actually, it does. It’s a rather fundamental concept we humans have of a fair exchange of value.

@ljharb
Copy link
Collaborator

ljharb commented May 21, 2019

No, that concept requires previous agreement. You can’t obligate someone to do work for you just because you decided to do work for them of your own accord, without their cooperation or remuneration or agreement.

What you’re describing is called entitlement, not a fair exchange.

@herebebeasties
Copy link

herebebeasties commented May 21, 2019

@ljharb

Speaking as a maintainer myself, the age of an issue does not have any correlation whatsoever to its priority, its risks, or the speed of addressing it.

No, quite right. I didn't say it did.
However, I did spell out the low risk of addressing this, the low time impact of reviewing it and the large number of people in the NPM community who are asking for it to be fixed.

When an issue that is literally a trivial three line fix and that has hundreds of people clamouring for you to land the community-provided (!) PR for it, is ignored entirely for four years, and maintainers are spending longer fending off people who are complaining about it rather than just landing the bloody thing, you surely have to concede that you are doing something wrong.

You can trot out the "it's free, you have no right to complain" line if you want to, but you are seriously losing mindshare and goodwill from a community that has already provided a fix for this for you, not once, but about four times over, and just been ignored and ignored for years.

It does not help your cause to then complain at these same people who have provided a fix. It's toxic behaviour and rightly makes them feel pissed.

@herebebeasties
Copy link

What you’re describing is called entitlement, not a fair exchange.

Actually, what he is describing is how healthy open source projects work.

I think we would all appreciate it if you could avoid using such personal terminology as "entitlement" (which to me seems totally unwarranted in this case - he provided the patch, which is pretty much the opposite) and concentrated on the actual issue here, which is getting this thing fixed.

In half the time spent antagonising each other on this issue, it could easily have been reviewed and landed.

@ljharb
Copy link
Collaborator

ljharb commented May 21, 2019

@herebebeasties my comment wasn’t directed at you; and the health of an open source project is not generically measured by how fast PRs are merged, nor which are merged. I understand their frustration, but “let npm burn” is not a productive or warranted sentiment to express.

Note; I’m not a maintainer here - there aren’t maintainers “fending off” anyone. I’m just a maintainer of other projects responding to the unjustified vitriol, even if it comes from understandable frustration.

@aeschright
Copy link
Contributor

Unfortunately, since we were let go from npm there's not much I can do about whether this PR gets merged or released. It's very difficult to make progress when there are no resources. I know this has been frustrating for cli users and contributors, but please be aware that the developers who've worked on this project have no control over what the company is doing now.

@jacksonrayhamilton
Copy link

I understand their frustration, but “let npm burn” is not a productive or warranted sentiment to express.

What doesn’t kill them can make them stronger. Otherwise, space can be made for a new default package manager for Node, or space can be made for new management for the NPM CLI that ships with Node. As a community we shouldn’t be complacent with the current state of affairs.

I can certainly sympathize with the predicament @aeschright describes. Responsibility travels up to NPM’s management and owners for prioritizing open source work. Are they up to that task? If not, NPM users should take their business elsewhere.

@icavalheiro
Copy link

Please merge this T-T
Those fsevents warning have been haunting me for years now, on my windows and linux work machines!

@tomcanham
Copy link

Ditto on "please merge" -- I try to get clean builds, and the warning (for fsevents in my case) is annoying.

@andiemmadavieswilcox
Copy link

andiemmadavieswilcox commented Jul 12, 2019

BUMP! This is getting ridiculous, I want my clean build damnit 😢 The best workaround I've found so far is as follows, but it's limited in that it means if you have genuine optional dependencies in your project, then they'll always be ignored (works fine for me as I don't have any):

package.json:

{
  "optionalDependencies": {
    "fsevents": "*"
  }
}

Install with:
npm install --no-optional

@NishealJ
Copy link

NishealJ commented Aug 5, 2019

is this expected to fix npm/npm#18712 ?

@spaceneenja
Copy link

It baffles me to think of the literal days which humanity has collectively spent on this issue. Something that seems to be easily fixed with a few lines of code. How many questions, GitHub issues, pull requests, google searches, etc... that this has caused?

NPM is a powerful tool but increasingly it's looking like it's the wrong package manager to be automatically included with Node. There are alternatives which take developer experience more seriously and don't offer up blase responses to concerns provided by the community.

@jacksonrayhamilton
Copy link

NPM is a powerful tool but increasingly it's looking like it's the wrong package manager to be automatically included with Node.

Yep. Maybe Node’s maintainers should fork the CLI.

Since people expect an npm program to be installed it might be tricky to replace NPM with Yarn or pnpm; there would have to be a compatibility layer that would turn npm install into a yarn install, convert between lockfile formats, etc.

@Pointotech
Copy link

This is insane. Why does lowering the priority of some garbage logs, that are by definition irrelevant to the computer that you're running them on, take half a decade to resolve? Just for political reasons, even though the code fix was provided years ago?

This is legislation by inaction that makes the federal government look fast and responsive.

@Pointotech
Copy link

And people are yelling at the person who supplied the fix, for suggesting that maybe SEVERAL YEARS is too long to keep a trivial PR open (after having their previous PR declined by a bot).

No one wants to contribute to a project that can't merge a trivial PR after years of discussion and hundreds of users asking for it.

@jacksonrayhamilton
Copy link

@Pointotech I dunno if it’s political. My guess is that the managers at NPM don’t realize that there are people who (formerly) would take time out of their day to improve their product for free simply because they want it to be better, and thus they never schedule time to work through open PRs. Gosh, you’d think: prioritizing reviewing already-written changes that users want ahead of other things like writing new changes from scratch.

It’s a real shame that in the last four years by inaction they have demonstrated that they still haven’t realized this. It’s especially bizarre considering they upload their software to a FOSS social network. And rather deceptive, I feel; by ostensibly “accepting PRs” to their repositories, they establish an expectation that this is a commune in which a fair exchange of value for the submission of a PR is a timely response of feedback/rejection/acceptance. Otherwise, contributors leave the commune, like I have.

Except, despite that I will not be writing code for NPM nor using their CLI, I’m still posting here because I care about Node, and the NPM CLI is knotted into Node. So if NPM being acquired by a trillion dollar company is still insufficient to get PRs merged, I guess our only hope is to change maintainership out from underneath them. But Node doesn’t seem to want to do that either, so I guess it will just suck.

@ljharb
Copy link
Collaborator

ljharb commented May 14, 2020

A lot of work has been put into npm 7 to resolve the many complex (not trivial) issues with optional deps and logging; just because this PR hasn’t seen movement doesn’t mean the problem is being ignored.

Nothing about FOSS guarantees any PR will be accepted; i think your expectations are set incorrectly.

@darcyclarke
Copy link
Contributor

@cmedinasoriano sorry for the delay here. I'm closing this as we don't print optional dep info/warnings today in v7 & the original issue referenced has also been closed. Appreciate the contribution though.

@markuszeller
Copy link

Warnings should be reduced to a minimum and only be given on a non-optional dependency. Seeing always so many warnings attracts unneeded attention.

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@2.1.2 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@2.1.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.13 (node_modules/webpack-dev-server/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.13: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.13 (node_modules/watchpack-chokidar2/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.13: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

@karikera
Copy link

I don't know why should I be warned even using normal working modules.

@JavierLight
Copy link

Does the npm dev team ever heard of the concept "normalization of deviance"?

This should never appear as a warning!!

@Pointotech
Copy link

I switched to Yarn on all of my projects because of the ridiculous dysfunction that this thread demonstrates. Haven't seen an fsevents warning since :)

@bibipkins
Copy link

its been 2 years since I last visited this thread. still not fixed and I still have to see this garbage in my build logs...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Info, rather than Warn, when skipping packages not suitable to target OS