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

fix: remove npm #418

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

darcyclarke
Copy link
Member

@darcyclarke darcyclarke commented Mar 7, 2024

This PR removes npm from Corepack. npm was supposed to be left out of Corepack & many people were/still are under the impression Corepack would not touch npm (although corepack enable npm has persisted); This PR makes it clear only yarn & pnpm will be managed & distributed by Corepack (likely superseding #407).

Fixes: #210, #419

@nodejs/npm

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I don't use npm, but when I do, it's through Corepack. I don't think we gain anything from this.

merceyz
merceyz previously requested changes Mar 7, 2024
sources/corepackUtils.ts Outdated Show resolved Hide resolved
@arcanis
Copy link
Contributor

arcanis commented Mar 7, 2024

I don't use npm, but when I do, it's through Corepack. I don't think we gain anything from this.

This is certainly going to be a loss of functionality for some people, but Corepack shouldn't include projects against the wish of their maintainers. It can be added back at a later time in a separate discussion which will involve the npm team if we find a common ground, but that's not the case today, and not something we pursue for bringing Corepack to stable.

@darcyclarke I'm generally +1, but please address @merceyz's other concern and get formal approval from someone from the actual npm team. As far as I'm aware you're working on a competing product, which makes you kind of the wrong person to make this request (part of why I didn't merge my own #407).

@aduh95
Copy link
Contributor

aduh95 commented Mar 7, 2024

Corepack shouldn't include projects against the wish of their maintainers

I disagree, I think users should be in control. We should respect the license chose by the author of the software, and AFAICT Corepack use is perfectly within the bounds of npm-cli license: https://github.com/npm/cli/blob/af3c48e074d03caebaa8ed24d39405329f545497/LICENSE#L105C1-L112C30 EDIT: as pointed out by Michaël, we do not distribute the npm source, we download the tarball from the npm registry.

@targos
Copy link
Member

targos commented Mar 7, 2024

I agree with @aduh95 but I don't think this section of the license is relevant. Corepack doesn't include or distribute npm's source code.

@arcanis
Copy link
Contributor

arcanis commented Mar 7, 2024

I disagree, I think users should be in control. We should respect the license chose by the author of the software

My line of thinking was that some basic level of support should be preferred, as it impacts the developer experience of the users of those tool (for instance, taking an extreme example, I'd certainly dislike it very much if the corepack keyword was expected to be added in front of every yarn command - it'd compromise our devx).

With that said I also understand your point - as far as I know Debian / Ubuntu / etc don't request permission to include softwares into their package lists (including npm and yarn), and our teams have obviously no control on how should apt be implemented, so perhaps the situation is similar here.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

I am reading that comment differently than you.

My request would be that if corepack is enabled by default that npm support remain behind an additional flag or command that would be opt-in for developers.

This is exactly how corepack works today so no need to remove npm.

-1 on this PR

Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

That's a good point, it's a fairly clear statement that the current setup is satisfying enough to the npm team. -1 for me then.

@darcyclarke
Copy link
Member Author

As far as I'm aware you're working on a competing product, which makes you kind of the wrong person to make this request (part of why I didn't merge my own #407)

I didn't realize there was extra criteria for contributing to this project. Who is "the right person"?

I'm sad you're questioning either my ethics, experience or insight in regards to even just proposing a change.

To be clear, it seemed less-ethical to propose changes to Corepack while I was still managing npm (@nodejs/npm can speak for themselves since I'm no longer affiliated).

Objectively, I'm actively working against my own interests (ie. building a startup) spending time here/contributing. I could be doing anything else & am instead sticking my neck out because others aren't willing to engage here. It would be better for me to stay back & let the chaos continue without trying to advocate for a better experience for end-users (myself included) with actual code changes.

Using affiliation to discredit contributions discourages people from engaging & can easily be turned around on you (ex. "were you the 'right person' to propose adding yarn or Corepack to core?" or "are you the 'right person' to merge your own PRs?" - you don't have to answer those because I think they're unfair & believe you have good intentions - but everyone has bias)

Removing npm properly should have been done years ago. It can always be added back.

@aduh95
Copy link
Contributor

aduh95 commented Mar 7, 2024

it seemed less-ethical to propose changes to Corepack while I was still managing npm

FWIW I don't think there's anything ethically wrong with npm folks contributing to Corepack, quite the opposite.

@melroy89
Copy link

melroy89 commented Mar 12, 2024

I don't use npm, but when I do, it's through Corepack. I don't think we gain anything from this.

Agreed. We just should make a split between runtime and package manager.

EDIT: Also Corepack takes care of auto installing the package manager. So it make sense to keep npm part of Corepack.

Copy link
Member

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

It's the position of the npm team that we do not think it is within our purview to dictate how a 3rd party tool interacts with or manages the npm CLI on a developers system when fetched dynamically.

The only request we've had to Corepack at this time is to not enable npm by default as it would complicate distribution via Node.js. If Corepack were not distributing with Node.js we would have 0 requests to the Corepack team regarding the management of local npm installation.

corepack enable npm won't be an officially supported install path for npm, but users should be able to run this command on their own system if they want Corepack to manage npm.

@ruyadorno
Copy link
Member

@lukekarrys if I understand well your comment an important question for the @nodejs/tsc here is whether we would be ok with enabling an unsupported install path for npm in node core?

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

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

@merceyz
Copy link
Member

merceyz commented Mar 27, 2024

This PR makes it clear only yarn & pnpm will be managed & distributed by Corepack

Note that if this PR lands it will most likely only make using npm through Corepack inconvenient, not impossible, since URLs are supported #359.

ronag
ronag previously approved these changes Mar 28, 2024
@ronag ronag dismissed their stale review March 28, 2024 11:16

Not sure if this is actually needed.

@ronag
Copy link
Member

ronag commented Mar 28, 2024

@darcyclarke or anyone else in favor of this PR. Could you please address the following objection:

... it's a fairly clear statement that the current setup is satisfying enough to the npm team ...

Seems to me that the motivation for this PR is not valid? Or am I missing something?

@styfle
Copy link
Member

styfle commented Mar 28, 2024

I see two responses from the npm team from two different people that both claim corepack enable npm is fine.

My request would be that if corepack is enabled by default that npm support remain behind an additional flag or command that would be opt-in for developers.
MylesBorins

corepack enable npm won't be an officially supported install path for npm, but users should be able to run this command on their own system if they want Corepack to manage npm.
lukekarrys

I must be missing something 🤔

@darcyclarke
Copy link
Member Author

darcyclarke commented Mar 28, 2024

@ronag I took @lukekarrys' response (on behalf of the npm team) to be extremely diplomatic & yet contradictory.

... users should be able to run this command on their own system if they want Corepack to manage npm.

This is pretty clear, the npm team doesn't care how users install & manage npm.

corepack enable npm won't be an officially supported install path for npm ...

This is pretty clear, the npm team doesn't want to support users that install & manage npm with corepack.

The question this leaves for me (& everyone here should ask themselves) is why we care about & want to maintain an install path for npm when they do not want it for themselves or support it for their end-users? This response & outlook is in stark contrast to the other projects (ie. yarn & pnpm) that are in full support of this project & method of installation.

My stance remains that npm should not be included in this experiment (as it was expressed to everyone it would not be) as we should not be burdening ourselves with supporting an unofficial/unsupported installation method for npm (as has been identified & reiterated by that team many many times).

If objections remain, this PR should be voted on by the TSC (afaict, it can happen async of any other discussion about corepack's future)

@nodejs/tsc

@darcyclarke
Copy link
Member Author

darcyclarke commented Mar 28, 2024

One other important note in regards to licensing (which @aduh95 / @targos commented on): I believe the specific provision which should be reviewed & understood by legal counsel (to ensure Corepack is compliant) is actually 4. ("Distribution of Modified Versions of the Package as Source ref. https://github.com/npm/cli/blob/af3c48e074d03caebaa8ed24d39405329f545497/LICENSE#L120-L137)

...

    "Standard Version" refers to the Package if it has not been
    modified, or has been modified only in ways explicitly requested
    by the Copyright Holder.

    "Modified Version" means the Package, if it has been changed, and
    such changes were not explicitly requested by the Copyright
    Holder.

...
    
Distribution of Modified Versions of the Package as Source

(4)  You may Distribute your Modified Version as Source (either gratis
or for a Distributor Fee, and with or without a Compiled form of the
Modified Version) provided that you clearly document how it differs
from the Standard Version, including, but not limited to, documenting
any non-standard features, executables, or modules, and provided that
you do at least ONE of the following:

    (a)  make the Modified Version available to the Copyright Holder
    of the Standard Version, under the Original License, so that the
    Copyright Holder may include your modifications in the Standard
    Version.

    (b)  ensure that installation of your Modified Version does not
    prevent the user installing or running the Standard Version. In
    addition, the Modified Version must bear a name that is different
    from the name of the Standard Version.

    (c)  allow anyone who receives a copy of the Modified Version to
    make the Source form of the Modified Version available to others
    under

        (i)  the Original License or

        (ii)  a license that permits the licensee to freely copy,
        modify and redistribute the Modified Version using the same
        licensing terms that apply to the copy that the licensee
        received, and requires that the Source form of the Modified
        Version, and of any works derived from it, be made freely
        available in that license fees are prohibited but Distributor
        Fees are allowed.

I am not a lawyer, but Corepack distributing an npm binary (ie. a "Modified Version") which blocks the installation/running of the "Standard Version" (ie. the real npm) & has not changed it's name, seems to be in violation of this license. If it continued to - & this PR was overturned - we'd likely need to comply with 4.a or 4.c & whether or not we are already compliant within the bounds of 4.c.ii. is, again, a question for a lawyer (ie. can npm Inc./GitHub modify & redistribute Corepack using the same license they distribute npm with, under the current license in use today?).

Both the OpenJS Foundation & GitHub's own lawyers should probably look at this licensing & implementation to ensure there are no violations (especially given that the whole intention of this implementation is to impersonate another trademarked software).

@bensternthal
Copy link

Hi folks, OpenJS will bring in our legal counsel to provide an opinion on Darcy's license question.

@styfle
Copy link
Member

styfle commented Mar 29, 2024

I am not a lawyer, but Corepack distributing an npm binary (ie. a "Modified Version")

In what way does Corepack modify npm? Isn't it installing exactly whats on the registry?

@darcyclarke
Copy link
Member Author

darcyclarke commented Mar 29, 2024

In what way does Corepack modify npm?

I am not a lawyer & I expect the OpenJS Foundation's legal counsel to provide an authoritive interpretation/response as @bensternthal notes above (not sure if GitHub will also provide their interpretation or engage their lawyers).

That said, from my perspective - as an engineer/end-user - when Corepack places a npm binary on your system (which is not npm's "Standard Version"/source) it is distributing a "Modified Version" (this interpretation may or may not be shared by others/counsel).

If we chose a different name then "npm" for the binary then you could say it was a different thing but this project intentionally names these binaries the same as the respective sources in order to intercept calls to them &, subsequently, execute or prevent the execution of the respective source/"Standard Version" (ref. see the behavior defined for COREPACK_ENABLE_STRICT - which is on by default - which gates the installation/execution of respective sources based on a project's packageManager field).

npm's license was drafted in such a way to explicitly protect against scenarios like this where software would mimic/impersonate/copy it while at the same time preventing access to it's upstream source. I do believe Corepack is likely violating the license, if not the spirit of the license (again, this is my opinion/interpretation & may not be shared).

The installation of npm's "Standard Version" post-validation is not in question. The question is whether Corepack's distribution of a "Modified Version" of npm, mechanism to prevent access to npm's "Standard Version" or license itself violate npm's licensing or not (ie. see the specific terms I previously referenced, there may also be other relevant terms I'm not aware of - again, ianal).

Notably, whether it's deemed to be in violation or not, my previous reasoning & support for this PR remains.

@isaacs
Copy link

isaacs commented Mar 29, 2024

I am not a lawyer. And, any IP rights I might have to npm's source code were long ago handed over to npm, Inc. and then subsequently to GitHub, so I don't have standing here.

But, my belief is that including the npm cli in corepack is indeed a violation of the letter of the license, and I am 100% confident that it is a violation of the spirit of the license.

The Artistic License 2.0 as chosen very deliberately to disallow distribution of a modified form of "npm" that was being distributed in Homebrew: jumper binaries that download source, wrap the original version, hard-code configurations, and so on. Essentially, exactly what corepack is doing.

Like I said, I'm not a lawyer, and don't have standing, since I sold my rights to GitHub. But for what it's worth, I am angry and disappointed to learn that npm is being distributed in this way, when I released it under a license that specifically disallowed this. At the very least, you should check with your own lawyers and possibly any contributors who didn't sign away their IP, since their objections may hold legal weight.

If you want to include npm in corepack, please call it something else other than "npm", so that it's clear that it's not the actual thing. Doing otherwise is misleading and unethical.

@ronag
Copy link
Member

ronag commented Mar 31, 2024

Adding to tsc agenda due to the legal concerns raised.

@mcollina
Copy link
Member

mcollina commented Apr 1, 2024

We should just remove it, as quickly as possible. I don't
think spending money on lawyers is worth it here.

Thanks @darcyclarke and @isaacs for bringing this up.

@isaacs
Copy link

isaacs commented Apr 1, 2024

To be 100% clear: I am specifically not making a legal threat or claim of any sort. I can't. Even if it was a very clear violation of the license, I don't own any of the IP licensed under Artistic 2.0. If GitHub says ok, then it's allowed. (Edit to add: also any other contributors who were not GitHub employees or otherwise signed over their rights, of course.)

I am saying that it makes me feel some kind of way, as the author who released it under that license for that purpose. Just a human having emotions. Maybe that's not enough to outweigh other considerations (and legally, it isn't even relevant at all), but I'd still consider it a data point if I was on the other side of this.

@styfle
Copy link
Member

styfle commented Apr 2, 2024

This would be a sad day for developers if this was a violation of the license.

Something like alias npm="socket npm" might also be a violation, if true.

https://socket.dev/blog/introducing-safe-npm

@rginn
Copy link

rginn commented Apr 2, 2024

In initially reviewing the license and related issues with our OpenJS legal counsel, it appears that there is no clear legal answer. If we decided to continue pursuing this investigation with our legal counsel, we would need to formally bring in independent technical experts given the interpretive nature of the issue, according to our attorney.

Unfortunately, the OpenJS Foundation cannot do a legal investigation of how all our projects interact with all third-party software - we often don’t have the rights or resources for that ongoing responsibility. We do support our project communities with legal support, such as trademarks, government compliance, and policy, but again with limited resources.

And as an umbrella organization, we don’t make technical decisions for our projects. In this case, the Node.js TSC is clearly autonomous to make technical decisions on what’s best for the project. I’m encouraged by these open discussions and advocate for an alternative technical approach working collaboratively with these project communities, and our CPC and Collaboration Spaces when possible.

@darcyclarke
Copy link
Member Author

darcyclarke commented Apr 2, 2024

Something like alias npm="socket npm" might also be a violation, if true.

Creating an alias or developing something for personal use on your own machine is not the same as distributing/redistributing modified software, links or interfaces.

In the above, creating an alias yourself would be fine but depending on how you got socket npm & what it does may or may not indeed violate the license.

@mcollina
Copy link
Member

mcollina commented Apr 4, 2024

Based on my understanding of the technology, because npm is not enabled by default the legal argument is a gray area. I don’t think we would be able to enable it by default either. Nor we plan to, at least on Node.js core.

With my Board hat on, I would say there is little legal risk to keeping this as is. It's not worth litigating this issue with foundation attorneys, which would run up large legal costs. It's better to solve this for what's the best technical decision for the developer experience.

@jasnell
Copy link
Member

jasnell commented Apr 7, 2024

I'm not going to weigh in on any legal issue. What I would comment on is that there doesn't seem to be any real consideration in this conversation about what is in our user's best interest. What makes user's lives easier or harder is a much more interesting conversation. I'm not going to offer an opinion one way or the other on this particular PR but it would be nice if this conversation could be more productively focused.

@GeoffreyBooth
Copy link
Member

What makes user's lives easier or harder is a much more interesting conversation.

If Node unbundles Corepack then it can do whatever it wants, including enabling support for npm by default, without any concerns from us. Of course this entails a tradeoff, in that users would run npm install -g corepack instead of npm enable corepack, but in return they'd get a much more powerful Corepack that has no restrictions imposed on it by Node.

@isaacs
Copy link

isaacs commented Apr 8, 2024

@jasnell

What I would comment on is that there doesn't seem to be any real consideration in this conversation about what is in our user's best interest.

Fwiw, the original choice to say "you are not allowed to ship not-npm and call it npm" is almost entirely motivated by user confusion and the frustration of being unable to support people who came to me with bugs that I could not fix. That's a terrible user experience that I observed repeatedly back then, and it's one that I think is inevitable when software is overly magical in ways that the authors do not anticipate.

I say "almost" entirely motivated because of course there's also the reputational impact of those people thinking I'd created bugs that I hadn't, and that's a bit more self-serving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shim doesn't work for npm when npm is installed with brew