X Tutup
The Wayback Machine - https://web.archive.org/web/20220130212109/https://github.com/nodejs/node/pull/39288
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

build: restore libplatform headers in distribution #39288

Closed
wants to merge 1 commit into from

Conversation

@jeroen
Copy link
Contributor

@jeroen jeroen commented Jul 6, 2021

I maintain libnode/v8 bindings for the R programming language, which depend on libnode on most Linux distro's, e.g. r-cran-v8 in Debian/Ubuntu and R-V8 in Fedora.

Headers considered non-essential were removed in #37570, however the libplatform API is actually needed by external software initiating the v8 engine. See for example the upstream v8 hello world example: https://chromium.googlesource.com/v8/v8/+/refs/heads/main/samples/hello-world.cc

I have also asked upstream v8 if the API in question could maybe be moved to the top level v8-* headers, but they have not responded.

I hope it can be reconsidered to ship these headers, otherwise it will no longer be possible for us to build bindings for libnode, which would be very unfortunate, because they are widely used.

Headers considered non-essential were removed in nodejs#37570, however the libplatform API is actualy needed (and used) by external software initiaing the v8 engine, see for example https://chromium.googlesource.com/v8/v8/+/refs/heads/main/samples/hello-world.cc
@richardlau
Copy link
Member

@richardlau richardlau commented Jul 6, 2021

@targos
Copy link
Member

@targos targos commented Jul 6, 2021

otherwise it will no longer be possible for us to build bindings for libnode, which would be very unfortunate, because they are widely used.

It's always possible for you to have your own copy of these headers.

That said, as the one who removed them and who does most of the work related to V8 updates, I'm okay with this change. The libplatform headers rarely have breaking changes.

targos
targos approved these changes Jul 6, 2021
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jul 6, 2021

Copy link
Member

@mhdawson mhdawson left a comment

LGTM

cjihrig
cjihrig approved these changes Jul 8, 2021
targos added a commit that referenced this issue Jul 11, 2021
Headers considered non-essential were removed in #37570, however the
libplatform API is actualy needed (and used) by external software
initiaing the v8 engine, see for example:
https://chromium.googlesource.com/v8/v8/+/refs/heads/main/samples/hello-world.cc

PR-URL: #39288
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos
Copy link
Member

@targos targos commented Jul 11, 2021

Landed in c476bb8

@targos targos closed this Jul 11, 2021
targos added a commit that referenced this issue Jul 11, 2021
Headers considered non-essential were removed in #37570, however the
libplatform API is actualy needed (and used) by external software
initiaing the v8 engine, see for example:
https://chromium.googlesource.com/v8/v8/+/refs/heads/main/samples/hello-world.cc

PR-URL: #39288
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jeroen jeroen deleted the libplatform-headers branch Jul 11, 2021
codebytere added a commit to electron/electron that referenced this issue Jul 14, 2021
jkleinsc added a commit to electron/electron that referenced this issue Jul 15, 2021
* chore: bump node in DEPS to v16.4.2

* chore: update patches

* ci: run main and remote woa tests separately

* chore: bump node in DEPS to v16.5.0

* build: restore libplatform headers in distribution

nodejs/node#39288

* build: pass directory instead of list of files to js2c.py

nodejs/node#39069

* chore: various BoringSSL/OpenSSL upstreams

- nodejs/node#39136
- nodejs/node#39138
- nodejs/node#39054

* test: move debugger test case to parallel

nodejs/node#39300

* chore: fixup patch indices

* build: pass directory instead of list of files to js2c.py

nodejs/node#39069

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
BlackHole1 added a commit to BlackHole1/electron that referenced this issue Aug 30, 2021
* chore: bump node in DEPS to v16.4.2

* chore: update patches

* ci: run main and remote woa tests separately

* chore: bump node in DEPS to v16.5.0

* build: restore libplatform headers in distribution

nodejs/node#39288

* build: pass directory instead of list of files to js2c.py

nodejs/node#39069

* chore: various BoringSSL/OpenSSL upstreams

- nodejs/node#39136
- nodejs/node#39138
- nodejs/node#39054

* test: move debugger test case to parallel

nodejs/node#39300

* chore: fixup patch indices

* build: pass directory instead of list of files to js2c.py

nodejs/node#39069

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
X Tutup