src: fix `ssize_t` error from `nghttp2.h` #44393

RaisinTen · 2022-08-25T12:48:55Z

The "node_http2.h" include reordering enforced by clang-format (32446d8#diff-33f026e43570112875cf4c8eab6743496f3aa014329611128e348ec23d6f771cR1)
broke Electron's Node.js upgrade on Windows.

https://ci.appveyor.com/project/electron-bot/electron-x64-testing/builds/44562886/job/vf7dodmpnfn47y68

In file included from ../../third_party/electron_node/src/node_http2.cc:1:
In file included from ../../third_party/electron_node/src/node_http2.h:8:
../../third_party/electron_node/deps/nghttp2/lib/includes\nghttp2/nghttp2.h(930,9): error: unknown type name 'ssize_t'
typedef ssize_t (*nghttp2_data_source_read_callback)(
        ^

ssize_t is a part of the POSIX standard and it's not available on
Windows, so the fix for this is to include "node.h" which
typedefs it on Windows in

node/src/node.h

Lines 212 to 220 in bb4dff7

    
           #ifdef _WIN32 
        
           #if !defined(_SSIZE_T_) && !defined(_SSIZE_T_DEFINED) 
        
           typedef intptr_t ssize_t; 
        
           # define _SSIZE_T_ 
        
           # define _SSIZE_T_DEFINED 
        
           #endif 
        
           #else  // !_WIN32 
        
           # include <sys/types.h>  // size_t, ssize_t 
        
           #endif  // _WIN32

.

Refs: electron/electron#35350 (comment)
Signed-off-by: Darshan Sen raisinten@gmail.com

cc @nodejs/cpp-reviewers

nodejs-github-bot · 2022-08-25T12:48:59Z

Review requested:

mcollina

lgtm

nodejs/node#44393

addaleax

Let’s not start duplicating multiple definitions of ssize_t in internal headers.

src/node_http2.h

The "node_http2.h" include reordering enforced by clang-format broke Electron's Node.js upgrade on Windows. ssize_t is a part of the POSIX standard and it's not available on Windows, so the fix for this is to include "node.h" which typedefs it on Windows in https://github.com/nodejs/node/blob/bb4dff783ddb3b20c67041f7ccef796c335c2407/src/node.h#L212-L220. Refs: electron/electron#35350 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>

src/node_http2.h

Co-authored-by: Anna Henningsen <github@addaleax.net>

nodejs-github-bot · 2022-08-27T09:47:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46270/

nodejs/node#44393

nodejs-github-bot · 2022-08-29T09:58:55Z

Landed in 72aafbd

* chore: bump node in DEPS to v16.17.0 * chore: fixup asar patch * lib: use null-prototype objects for property descriptors nodejs/node#43270 * src: make SecureContext fields private nodejs/node#43173 * crypto: remove Node.js-specific webcrypto extensions nodejs/node#43310 * test: refactor to top-level await nodejs/node#43500 * deps: cherry-pick two libuv fixes nodejs/node#43950 * src: slim down env-inl.h nodejs/node#43745 * util: add AggregateError.prototype.errors to inspect output nodejs/node#43646 * esm: improve performance & tidy tests nodejs/node#43784 * src: NodeArrayBufferAllocator delegates to v8's allocator nodejs/node#43594 * chore: update patch indices * chore: update filenames * src: refactor IsSupportedAuthenticatedMode nodejs/node#42368 * src: add --openssl-legacy-provider option nodejs/node#40478 * lib,src: add source map support for global eval nodejs/node#43428 * trace_events: trace net connect event nodejs/node#43903 * deps: update ICU to 71.1 nodejs/node#42655 This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093 * lib: give names to promisified exists() and question() nodejs/node#43218 * crypto: add CFRG curves to Web Crypto API nodejs/node#42507 * src: fix memory leak for v8.serialize nodejs/node#42695 This test does not work for Electron as they do not use V8's ArrayBufferAllocator. * buffer: fix atob input validation nodejs/node#42539 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

The "node_http2.h" include reordering enforced by clang-format broke Electron's Node.js upgrade on Windows. ssize_t is a part of the POSIX standard and it's not available on Windows, so the fix for this is to include "node.h" which typedefs it on Windows in https://github.com/nodejs/node/blob/bb4dff783ddb3b20c67041f7ccef796c335c2407/src/node.h#L212-L220. Refs: electron/electron#35350 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #44393 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

The "node_http2.h" include reordering enforced by clang-format broke Electron's Node.js upgrade on Windows. ssize_t is a part of the POSIX standard and it's not available on Windows, so the fix for this is to include "node.h" which typedefs it on Windows in https://github.com/nodejs/node/blob/bb4dff783ddb3b20c67041f7ccef796c335c2407/src/node.h#L212-L220. Refs: electron/electron#35350 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: nodejs#44393 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

The "node_http2.h" include reordering enforced by clang-format broke Electron's Node.js upgrade on Windows. ssize_t is a part of the POSIX standard and it's not available on Windows, so the fix for this is to include "node.h" which typedefs it on Windows in https://github.com/nodejs/node/blob/bb4dff783ddb3b20c67041f7ccef796c335c2407/src/node.h#L212-L220. Refs: electron/electron#35350 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #44393 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Aug 25, 2022

RaisinTen mentioned this pull request Aug 25, 2022

chore: bump node to v16.17.0 (main) electron/electron#35350

Merged

mcollina approved these changes Aug 25, 2022

View changes

RaisinTen force-pushed the fix-ssize_t-error branch 2 times, most recently from dc9bfca to 7d9a57f Compare Aug 25, 2022

codebytere added a commit to electron/electron that referenced this pull request Aug 25, 2022

src: fix ssize_t error from nghttp2.h

79cabe0

nodejs/node#44393

codebytere approved these changes Aug 25, 2022

View changes

RafaelGSS approved these changes Aug 25, 2022

View changes

RaisinTen force-pushed the fix-ssize_t-error branch from 7d9a57f to 742efa6 Compare Aug 25, 2022

addaleax requested changes Aug 26, 2022

View changes

src/node_http2.h Outdated Show resolved Hide resolved

RaisinTen force-pushed the fix-ssize_t-error branch from 742efa6 to c4f9a6e Compare Aug 26, 2022

RaisinTen requested a review from addaleax Aug 26, 2022

addaleax approved these changes Aug 26, 2022

View changes

src/node_http2.h Outdated Show resolved Hide resolved

src: resolve stdint.h FIXME

a6573ba

Co-authored-by: Anna Henningsen <github@addaleax.net>

RaisinTen requested a review from addaleax Aug 26, 2022

addaleax approved these changes Aug 26, 2022

View changes

RaisinTen added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 26, 2022

tniessen approved these changes Aug 26, 2022

View changes

RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 27, 2022

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Aug 28, 2022

CI Reliability 2022-08-28 nodejs/reliability#360

Open

30 tasks

jasnell approved these changes Aug 28, 2022

View changes

github-actions bot mentioned this pull request Aug 29, 2022

CI Reliability 2022-08-29 nodejs/reliability#361

Open

30 tasks

RaisinTen self-assigned this Aug 29, 2022

codebytere added a commit to electron/electron that referenced this pull request Aug 29, 2022

src: fix ssize_t error from nghttp2.h

0f89c67

nodejs/node#44393

RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2022

nodejs-github-bot merged commit 72aafbd into nodejs:main Aug 29, 2022
52 checks passed

RaisinTen deleted the fix-ssize_t-error branch Aug 29, 2022

This was referenced Aug 30, 2022

CI Reliability 2022-08-30 nodejs/reliability#362

Open

CI Reliability 2022-08-31 nodejs/reliability#363

Open

CI Reliability 2022-09-01 nodejs/reliability#364

Open

RafaelGSS mentioned this pull request Sep 5, 2022

v18.9.0 release proposal #44521

Merged

src: fix `ssize_t` error from `nghttp2.h` #44393

src: fix `ssize_t` error from `nghttp2.h` #44393

RaisinTen commented Aug 25, 2022 •

edited

nodejs-github-bot commented Aug 25, 2022

mcollina left a comment

addaleax left a comment

This comment was marked as outdated.

nodejs-github-bot commented Aug 27, 2022

nodejs-github-bot commented Aug 29, 2022

	#ifdef _WIN32
	#if !defined(_SSIZE_T_) && !defined(_SSIZE_T_DEFINED)
	typedef intptr_t ssize_t;
	# define _SSIZE_T_
	# define _SSIZE_T_DEFINED
	#endif
	#else // !_WIN32
	# include <sys/types.h> // size_t, ssize_t
	#endif // _WIN32

src: fix ssize_t error from nghttp2.h #44393

src: fix ssize_t error from nghttp2.h #44393

Conversation

RaisinTen commented Aug 25, 2022 • edited

nodejs-github-bot commented Aug 25, 2022

mcollina left a comment

addaleax left a comment

This comment was marked as outdated.

nodejs-github-bot commented Aug 27, 2022

nodejs-github-bot commented Aug 29, 2022

src: fix `ssize_t` error from `nghttp2.h` #44393

src: fix `ssize_t` error from `nghttp2.h` #44393

RaisinTen commented Aug 25, 2022 •

edited