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

Guide on introducing new dependencies in the codebase #16640

Closed
joyeecheung opened this issue Oct 31, 2017 · 11 comments
Closed

Guide on introducing new dependencies in the codebase #16640

joyeecheung opened this issue Oct 31, 2017 · 11 comments
Labels
doc Issues and PRs related to the documentations.

Comments

@joyeecheung
Copy link
Member

  • Version: master
  • Subsystem: doc

We have a guide on introducing new modules, and some guides(including WIP) on maintaining existing dependencies, but still need a guide on introducing one.

cc @jasnell @jkrems @watilde because IIRC you have introduced new dependencies to the code base in the past year, would love to get your help on this.

Refs: #16637 (comment)

@mscdex mscdex added the doc Issues and PRs related to the documentations. label Oct 31, 2017
@maclover7
Copy link
Contributor

👍 I was going to take a look at creating a general "Dependencies in Node" guide once #16541 lands, to have a section for introducing dependencies, and then also maintenance guides for each current dependency. There's already a maintenance guide for V8 in master.

@watilde
Copy link
Member

watilde commented Oct 31, 2017

This will be very helpful when we would like to add tools. Here are a couple of things what I cared when added the new deps.

  • Separate the commits into the actual integration and install node_modules.
  • Land the integration commit first to make the review a bit easier
  • To add node_modules, I used npm install --ignore-scripts to not include the native build modules and add npm rebuild script to Makefile.

@jkrems
Copy link
Contributor

jkrems commented Oct 31, 2017

In cases (?) where it's something that needs to work at runtime (e.g. because the code will be part of the distributed packages), there's also testing for both *nix and Windows. If the thing in question supports npm install && npm test, it can use tools/test-npm-package

@TimothyGu
Copy link
Member

To add more stuff to deps/ (for something like #15566), just unpack the tarball, and add an entry to tools/license-builder.sh.

@refack
Copy link
Contributor

refack commented Oct 31, 2017

  1. I would like to hear an expert opinion on licenses when adding tooling to /tools/. AFAICT that does not necessitate an explicit attribution as it should not be part of the runtime (a.k.a. the final product).

  2. npmed modules need special attention as they

    1. may need maintaining and updating
    2. may come with a substantial amount of files (1000s) which comes with a degraded git experiance

@richardlau
Copy link
Member

But we don't just distribute the runtime. As soon as we commit to this repository we've made a copy and are distributing (via GitHub). We only have the one LICENSE (i.e. we don't have one for the runtime binaries and one for the source).

Whether the attribution needs to be explicit depends on the terms of the license of the thing being added.

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 3, 2017

Hmm..I am not a lawyer but are those license files recursive? I mean we do attribute our dependencies in our own license, so other projects embedding Node.js don't have to attribute all of them again right? So it's gonna like, if our direct dependencies are doing proper licensing, then we don't have to worry about that? Because technically all the npm modules distribute nested node_modules as well, just not via github but via npm.

@apapirovski
Copy link
Member

apapirovski commented Nov 3, 2017

It's possible I'm missing something here but we do include the LICENSE files in the repo. I don't see any evidence that we need to put their contents into our own LICENSE.

Most of them just state "shall be included" and it is... Even something like ISC just states

Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above
copyright notice and this permission notice appear in all copies.

@apapirovski
Copy link
Member

apapirovski commented Nov 3, 2017

Reading Apache, GPL, etc. it seems like our main concern here would be mainly with GPL-3.0 which requires explicit mention within the distribution's own license file. So check if anything within node_modules is GPL and add it, I suppose. The rest should be fine as is.

List of currently included GPL, LGPL, etc. licensed modules:

  • jschardet

@mhdawson
Copy link
Member

mhdawson commented Nov 3, 2017

@apapirovski I think the GPL licenses require more than a mention and often have a "viral" component. I agree its important to have a list of licences that would be acceptable or not for inclusion.

@Trott
Copy link
Member

Trott commented Oct 26, 2018

Awesome if someone wants to write this thing, but it does seem like this conversation has stalled so I'm going to close this. Feel free to re-open if you disagree and feel it should not be closed.

@Trott Trott closed this as completed Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

No branches or pull requests