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.

Sign up for GitHub

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

Jump to bottom

deps,src: align ssize_t ABI between Node & nghttp2 #18565

Closed
wants to merge 1 commit into from
Closed

deps,src: align ssize_t ABI between Node & nghttp2 #18565

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 4, 2018

Previously, we performed casts that are considered undefined behavior.
Instead, just define ssize_t for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be technically
correct but shouldn’t matter as long as nobody is complaining.
(And is therefore unlikely to ever be implemented.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs/http2

CI: https://ci.nodejs.org/job/node-test-commit/15932/

@addaleax
deps,src: align ssize_t ABI between Node & nghttp2
af1e81f 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.
@addaleax addaleax added the http2 Issues or PRs related to the http2 subsystem. label Feb 4, 2018
@addaleax addaleax requested a review from jasnell February 4, 2018 19:22
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 4, 2018
mcollina
mcollina approved these changes Feb 5, 2018
View reviewed changes
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.

Would we have to float this patch on top of nghttp2 forever? Can we submit something upstream?

LGTM on the change itself.

@addaleax
Copy link
Member Author

addaleax commented Feb 5, 2018 •
edited
Loading

Would we have to float this patch on top of nghttp2 forever?

It’s not really a floating patch – config.h is an automatically generated file that we checked into our repository, it doesn’t exist upstream.

Can we submit something upstream?

I don’t have the links to the other issues where this was brought up at hand here. The “upstream” source of the arguably bogus #define ssize_t int is autotools itself, and submitting a patch there seems doable but also like a major breaking change for the whole autotools ecosystem, so I wouldn’t expect us to be able do anything about this.

We could do something like suggested in nghttp2/nghttp2#1008 and try to replace the usage of ssize_t in nghttp2 altogether. ¯\_(ツ)_/¯

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2018
@BridgeAR
Copy link
Member

Landed in 93bbe4e 🎉

@BridgeAR BridgeAR closed this Feb 10, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 10, 2018
@addaleax @BridgeAR
deps,src: align ssize_t ABI between Node & nghttp2
93bbe4e 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.

PR-URL: nodejs#18565
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@addaleax addaleax deleted the nghttp2-ssize_t branch February 21, 2018 19:00
@addaleax addaleax removed backport-requested-v9.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 27, 2018
addaleax added a commit to addaleax/node that referenced this pull request Feb 27, 2018
@addaleax
deps,src: align ssize_t ABI between Node & nghttp2
7fb72a5 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.

PR-URL: nodejs#18565
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax mentioned this pull request Feb 27, 2018
Closed
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
@addaleax @kjin
deps,src: align ssize_t ABI between Node & nghttp2
ab416d7 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.

PR-URL: nodejs#18565
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
@addaleax @MylesBorins
deps,src: align ssize_t ABI between Node & nghttp2
55cf145 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.

Backport-PR-URL: #20456
PR-URL: #18565
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@addaleax @MayaLekova
deps,src: align ssize_t ABI between Node & nghttp2
dbd59c6 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.

PR-URL: nodejs#18565
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
@addaleax @MylesBorins
deps,src: align ssize_t ABI between Node & nghttp2
1131241 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.

Backport-PR-URL: #20456
PR-URL: #18565
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
@addaleax @MylesBorins
deps,src: align ssize_t ABI between Node & nghttp2
5394bc5 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.

Backport-PR-URL: #20456
PR-URL: #18565
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@addaleax @BridgeAR @MylesBorins @mcollina @bnoordhuis @jasnell @cjihrig @nodejs-github-bot