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

Do we need to include any new licenses when we ship the new v8 inspector? #7123

Closed
MylesBorins opened this issue Jun 3, 2016 · 17 comments · Fixed by #7796
Closed

Do we need to include any new licenses when we ship the new v8 inspector? #7123

MylesBorins opened this issue Jun 3, 2016 · 17 comments · Fixed by #7796
Labels
inspector Issues and PRs related to the V8 inspector protocol

Comments

@MylesBorins
Copy link
Contributor

It does not appear that v8_inspector has a LICENSE but some of the vendored deps do including:

I do not believe we will have to as both of these appear to be build tools, but I'm not entirely sure and wanted to play it safe.

@ofrobots is there anything we should know license wise before shipping?

@mikeal thoughts?

/cc @nodejs/tsc

@mscdex mscdex added debugger inspector Issues and PRs related to the V8 inspector protocol and removed debugger labels Jun 3, 2016
@ofrobots
Copy link
Contributor

ofrobots commented Jun 3, 2016

v8_inspector is extracted from Blink (license). We can add a LICENSE file to it (/cc @pavelfeldman).

jinja2 and markup_safe are build time dependencies.

@rvagg
Copy link
Member

rvagg commented Jun 3, 2016

If you look in license-builder you'll see a section at the bottom for build tools, jinja2 and markup_safe can go there. Being able to pull LICENSE files in some form for both of these as well as v8_inspector in-tree would be ideal. Currently we only have to go out of tree for punycode and we can't properly match version for that when we do so it's not ideal.

@jasnell
Copy link
Member

jasnell commented Jun 3, 2016

@ofrobots ... can you verify that neither jinja2 or markup_safe insert any of their own licensed boilerplate or template code into the generated output during build time? If they do, then we may still need to include mention of those bits in the license file.

@ofrobots
Copy link
Contributor

ofrobots commented Jun 3, 2016

@jasnell jinja2 (website) is a simple python template engine (similar to handlebars or mustache). It is used to generate protocol files for v8_inspector. markup_safe is a dependency of jinja2.

Here's an example template file Backend_h.template that gets expanded to a C++ file Backend.h, or you can check out/Release/obj/gen/blink/platform/inspector_protocol/Backend.h after a Node build. As you can see no license boilerplate is being added.

@jasnell
Copy link
Member

jasnell commented Jun 3, 2016

Awesome. Thank you!
On Jun 3, 2016 4:16 PM, "Ali Ijaz Sheikh" [email protected] wrote:

@jasnell https://github.com/jasnell jinja2 (website
http://jinja.pocoo.org/docs/dev/) is a simple python template engine
(similar to handlebars or mustache). It is used to generate protocol files
for v8_inspector. markup_safe is a dependency of jinja2.

Here's an example template file Backend_h.template
https://github.com/nodejs/node/blob/d0151695a7a5504115dd3feb4ffac7557e9e31b2/deps/v8_inspector/platform/inspector_protocol/Backend_h.template
that gets expanded to a C++ file Backend.h
https://gist.github.com/ofrobots/ed138b69f8602640c04fa4d088c4e160, or
you can check
out/Release/obj/gen/blink/platform/inspector_protocol/Backend.h after a
Node build. As you can see no license boilerplate is being added.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7123 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eSGE3UVZ_aqFV5uJ_2Bc8h1uZGMBks5qILXKgaJpZM4ItHk8
.

@richardlau
Copy link
Member

This issue hasn't been resolved but the inspector shipped in Node.js v6.3.0.

Given that Blink's license is not the same as Node.js' primary license the v8_inspector really should have its own LICENSE since some of the source files say

// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

and at present there is no LICENSE in deps/v8_inspector.

@richardlau
Copy link
Member

With regards to dependencies

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;
;;; Commentary:
;;
;; Mostly ripped off django-mode by Lennart Borgman.
;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;
;; This program is free software; you can redistribute it and/or
;; modify it under the terms of the GNU General Public License as
;; published by the Free Software Foundation; either version 2, or
;; (at your option) any later version.
;;
;; This program is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
;; General Public License for more details.
;;
;; You should have received a copy of the GNU General Public License
;; along with this program; see the file COPYING.  If not, write to
;; the Free Software Foundation, Inc., 51 Franklin Street, Fifth
;; Floor, Boston, MA 02110-1301, USA.
;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 *  This library is free software; you can redistribute it and/or
 *  modify it under the terms of the GNU Library General Public
 *  License as published by the Free Software Foundation; either
 *  version 2 of the License, or (at your option) any later version.
 *
 *  This library is distributed in the hope that it will be useful,
 *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 *  Library General Public License for more details.
 *
 *  You should have received a copy of the GNU Library General Public License
 *  along with this library; see the file COPYING.LIB.  If not, write to
 *  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
 *  Boston, MA 02110-1301, USA.

@bnoordhuis
Copy link
Member

What is deps/v8_inspector/deps/wtf?

A utility library also used by other projects (WebKit uses it, as does Mozilla.)

I'm a bit surprised to see LGPL code in there. @ofrobots?

@pavelfeldman
Copy link
Contributor

pavelfeldman commented Jul 11, 2016

What is deps/v8_inspector/deps/wtf?

It should not be LGPL, I'll fix it. But more importantly, v8_inspector/deps/wtf is not a part of upstream, so this whole folder should not be there. PlatformWTF.h should have inlined compatibility layer declarations for wtf. I filed crbug.com/627114 upstream to track the inlining.

[EDIT:ofrobots: fix link]

@ofrobots
Copy link
Contributor

The deps/v8_inspector directory is a gathering of external dependencies, and no original content other than the README.md file (which documents this).

  • jinja and markupsafe are build time only tools.
  • code in deps/v8_inspector/platform/ tracks upstream repositories 1 and 2 which are subsets of the Blink (a fork of WebKit, still called WebKit in the chromium source). Note that the Blink license file has recently (3) merged with the chromium license 4. Note: I am not a lawyer.

@pavelfeldman
Copy link
Contributor

What is deps/v8_inspector/deps/wtf?

Ouch. It should have been inlined in PlatformSTL.h (for node) and it was, the deps/wtf is not used and should have been removed. We only pick two folders (inspector_protocol and v8_inspector) from upstream and don't add anything else into the build.

@ofrobots
Copy link
Contributor

Ouch. It should have been inlined in PlatformSTL.h (for node) and it was, the deps/wtf is not used and should have been removed.

This is my bad. #7302 should have deleted deps/wtf as part of restructuring. I will do so in PR soon.

@mhdawson
Copy link
Member

So from what I understand wtf will be removed so the remaining concern would be deps/v8_inspector/deps/jinja2/ext/jinja.el.

From earlier comments it sounds like it is only used to generate files from templates, which don't include the licence, and only these new files are used to create what is part of the delivered node packages. The result being that none of the files with the GPL license are "shipped" as part of the Node.js deliveries. @ofrobots is this correct ?

@bnoordhuis
Copy link
Member

jinja.el is an emacs syntax highlighting script. It's not used and can be removed without harm.

@ofrobots
Copy link
Contributor

Yes, deps/wtf will be removed in the next roll (I'm working on this). The jinja.el script is neither used in the build nor shipped as part of a node distribution.

@MylesBorins
Copy link
Contributor Author

@ofrobots when do you expect the next roll to come in? Is it safe to assume that it should resolve all licensing concerns?

ofrobots added a commit to ofrobots/node that referenced this issue Jul 15, 2016
Remove wtf files as v8_inspector no longer needs them.

Ref: nodejs#7123
@ofrobots
Copy link
Contributor

ofrobots commented Jul 15, 2016

These are the concerns that I am aware of:

targos pushed a commit that referenced this issue Jul 19, 2016
Remove wtf files as v8_inspector no longer needs them.

Ref: #7123

PR-URL: #7751
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
evanlucas pushed a commit that referenced this issue Jul 19, 2016
Remove wtf files as v8_inspector no longer needs them.

Ref: #7123

PR-URL: #7751
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
evanlucas pushed a commit that referenced this issue Jul 20, 2016
Remove wtf files as v8_inspector no longer needs them.

Ref: #7123

PR-URL: #7751
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
ofrobots added a commit to ofrobots/node that referenced this issue Jul 21, 2016
To add a LICENSE file along with the v8_inspector code, we need to
pick up v8_inspector from an intermediate repository:
https://github.com/pavelfeldman/v8_inspector. This repo still tracks
upstream code in Blink.

This roll also picks up the latest v8_inspector from upstream fixing
a few issues.

* Pickup commit id bc60957 from pavelfeldman/v8_inspector
* Update node.gyp to adapt to the new file paths
* Update the DevTools hash for the devtools frontend.

Fixes: nodejs#7123
Fixes: nodejs#7736
Fixes: nodejs#7734
ofrobots added a commit to ofrobots/node that referenced this issue Jul 27, 2016
To add a LICENSE file along with the v8_inspector code, we need to
pick up v8_inspector from an intermediate repository:
https://github.com/pavelfeldman/v8_inspector. This repo still tracks
upstream code in Blink.

This roll also picks up the latest v8_inspector from upstream fixing
a few issues.

* Pickup commit id bc60957 from pavelfeldman/v8_inspector
* Update node.gyp to adapt to the new file paths
* Update the DevTools hash for the devtools frontend.

Fixes: nodejs#7123
Fixes: nodejs#7736
Fixes: nodejs#7734
PR-URL: nodejs#7796
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
cjihrig pushed a commit that referenced this issue Aug 11, 2016
To add a LICENSE file along with the v8_inspector code, we need to
pick up v8_inspector from an intermediate repository:
https://github.com/pavelfeldman/v8_inspector. This repo still tracks
upstream code in Blink.

This roll also picks up the latest v8_inspector from upstream fixing
a few issues.

* Pickup commit id bc60957 from pavelfeldman/v8_inspector
* Update node.gyp to adapt to the new file paths
* Update the DevTools hash for the devtools frontend.

Fixes: #7123
Fixes: #7736
Fixes: #7734
PR-URL: #7796
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants