GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — deps: update nghttp2 to 1.51.0 by targos · Pull Request #45455 · nodejs/node

Conversation

@targos

@targos targos commented Nov 14, 2022

Copy link
Copy Markdown
Member

Refs: https://github.com/nghttp2/nghttp2/releases/tag/v1.51.0

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@targos
deps: update nghttp2 to 1.51.0
93cb6d1
Refs: https://github.com/nghttp2/nghttp2/releases/tag/v1.51.0
@nodejs-github-bot

nodejs-github-bot commented Nov 14, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Nov 14, 2022
@targos targos added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Nov 14, 2022
@targos

targos commented Nov 14, 2022

Copy link
Copy Markdown
Member Author

I used https://github.com/nodejs/node/blob/main/tools/update-nghttp2.sh but somehow config.h is removed while still being included by other files.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Nov 14, 2022

Copy link
Copy Markdown
Member Author

@yashLadha is the script missing a step?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ShogunPanda
ShogunPanda approved these changes Nov 14, 2022
View reviewed changes

@ShogunPanda ShogunPanda left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@targos

targos commented Nov 14, 2022

Copy link
Copy Markdown
Member Author

@ShogunPanda but it doesn't work!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ShogunPanda

ShogunPanda commented Nov 14, 2022

Copy link
Copy Markdown
Contributor

@targos I haven't executed it :) Changes on source code seemed reasonable, I was not really looking in the build script.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@yashLadha

yashLadha commented Nov 14, 2022

Copy link
Copy Markdown
Contributor

@yashLadha is the script missing a step?

No @targos last time I checked it worked completely fine.

Can you paste the log about what is the exact error. Might be due to recent changes in upstream dependencies.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Nov 14, 2022

Copy link
Copy Markdown
Member Author

You can check any of the failing builds here

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos targos added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Nov 15, 2022
@gengjiawen

gengjiawen commented Nov 15, 2022

Copy link
Copy Markdown
Member

@ShogunPanda but it doesn't work!

It's auto-generated, you need to copy it from build folder.
image

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gengjiawen

gengjiawen commented Nov 15, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Original config.h for v.1.51.0. But from the comment we may need to revert current one since the file comments like
/* Edited to match src/node.h. */

config.h
/* Hint to the compiler that a function never returns */
#define NGHTTP2_NORETURN __attribute__((noreturn))

/* Define to `int' if <sys/types.h> does not define. */
/* #undef ssize_t */

/* Define to 1 if you have the `std::map::emplace`. */
#define HAVE_STD_MAP_EMPLACE 1

/* Define to 1 if you have `libjansson` library. */
#define HAVE_JANSSON 1

/* Define to 1 if you have `libxml2` library. */
#define HAVE_LIBXML2 1

/* Define to 1 if you have `mruby` library. */
/* #undef HAVE_MRUBY */

/* Define to 1 if you have `neverbleed` library. */
/* #undef HAVE_NEVERBLEED */

/* sizeof(int *) */
#define SIZEOF_INT_P   8

/* sizeof(time_t) */
#define SIZEOF_TIME_T  8

/* Define to 1 if you have the `_Exit` function. */
#define HAVE__EXIT 1

/* Define to 1 if you have the `accept4` function. */
/* #undef HAVE_ACCEPT4 */

/* Define to 1 if you have the `mkostemp` function. */
#define HAVE_MKOSTEMP 1

/* Define to 1 if you have the `initgroups` function. */
#define HAVE_DECL_INITGROUPS 1

/* Define to 1 to enable debug output. */
/* #undef DEBUGBUILD */

/* Define to 1 if you want to disable threads. */
/* #undef NOTHREADS */

/* Define to 1 if you have the <arpa/inet.h> header file. */
#define HAVE_ARPA_INET_H 1

/* Define to 1 if you have the <fcntl.h> header file. */
#define HAVE_FCNTL_H 1

/* Define to 1 if you have the <inttypes.h> header file. */
#define HAVE_INTTYPES_H 1

/* Define to 1 if you have the <limits.h> header file. */
#define HAVE_LIMITS_H 1

/* Define to 1 if you have the <netdb.h> header file. */
#define HAVE_NETDB_H 1

/* Define to 1 if you have the <netinet/in.h> header file. */
#define HAVE_NETINET_IN_H 1

/* Define to 1 if you have the <pwd.h> header file. */
#define HAVE_PWD_H 1

/* Define to 1 if you have the <sys/socket.h> header file. */
#define HAVE_SYS_SOCKET_H 1

/* Define to 1 if you have the <sys/time.h> header file. */
#define HAVE_SYS_TIME_H 1

/* Define to 1 if you have the <syslog.h> header file. */
#define HAVE_SYSLOG_H 1

/* Define to 1 if you have the <time.h> header file. */
#define HAVE_TIME_H 1

/* Define to 1 if you have the <unistd.h> header file. */
#define HAVE_UNISTD_H 1

/* Define to 1 if HTTP/3 is enabled. */
/* #undef ENABLE_HTTP3 */

/* Define to 1 if you have `libbpf` library. */
/* #undef HAVE_LIBBPF */

/* Define to 1 if you have enum bpf_stats_type in linux/bpf.h. */
/* #undef HAVE_BPF_STATS_TYPE */

/* Define to 1 if you have `libngtcp2_crypto_openssl` library. */
/* #undef HAVE_LIBNGTCP2_CRYPTO_OPENSSL */
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gengjiawen
gengjiawen reviewed Nov 15, 2022
View reviewed changes
Comment thread deps/nghttp2/lib/includes/config.h
/* Hint to the compiler that a function never returns */
#define NGHTTP2_NORETURN

/* Edited to match src/node.h. */

@gengjiawen gengjiawen Nov 15, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More likely we need to revert this file than using the auto-gen one.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@targos

targos commented Nov 15, 2022

Copy link
Copy Markdown
Member Author

@yashLadha If I run the script with the current version (./tools/update-nghttp2.sh 1.47.0), the same problem happens (config.h is deleted):

$ git status
On branch main
Your branch is up to date with 'origin/main'.

nothing to commit, working tree clean

$ ./tools/update-nghttp2.sh 1.47.0
Making temporary workspace
Fetching nghttp2 source archive
Removing everything, except lib/ and COPYING
Copying existing gyp files
Replacing existing nghttp2
All done!

Please git add nghttp2, commit the new version:

$ git add -A deps/nghttp2
$ git commit -m "deps: update nghttp2 to 1.47.0"


$ git status
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        deleted:    deps/nghttp2/lib/includes/config.h

no changes added to commit (use "git add" and/or "git commit -a")
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@yashLadha

yashLadha commented Nov 19, 2022

Copy link
Copy Markdown
Contributor

Will check today, and close on this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@yashLadha

yashLadha commented Nov 19, 2022

Copy link
Copy Markdown
Contributor

I checked the 1.47.0 tag as well and there were no include.h file present in the tree, yet the builds passed #42127. I don't see anything changed in the script.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Nov 20, 2022
deps: update nghttp2 to 1.51.0 #45537
Merged
Script to update nghttp2 doesn't work #45572
Closed
@targos targos closed this Nov 22, 2022
@targos
targos deleted the nghttp2-1.51.0 branch November 22, 2022 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gengjiawen gengjiawen gengjiawen left review comments
@ShogunPanda ShogunPanda ShogunPanda approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@targos @nodejs-github-bot @ShogunPanda @yashLadha @gengjiawen
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.