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

deps: update nghttp2 to 1.51.0 #45455

Closed
wants to merge 1 commit into from
Closed

Conversation

targos
Copy link
Member

@targos targos commented Nov 14, 2022

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or 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. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 14, 2022
@targos
Copy link
Member Author

targos commented Nov 14, 2022

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.

@targos
Copy link
Member Author

targos commented Nov 14, 2022

@yashLadha is the script missing a step?

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@targos
Copy link
Member Author

targos commented Nov 14, 2022

@ShogunPanda but it doesn't work!

@ShogunPanda
Copy link
Contributor

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

@yashLadha
Copy link
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.

@targos
Copy link
Member Author

targos commented Nov 14, 2022

You can check any of the failing builds here

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

@ShogunPanda but it doesn't work!

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

@gengjiawen
Copy link
Member

gengjiawen commented Nov 15, 2022

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 */

/* Hint to the compiler that a function never returns */
#define NGHTTP2_NORETURN

/* Edited to match src/node.h. */
Copy link
Member

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.

@targos
Copy link
Member Author

targos commented Nov 15, 2022

@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")

@yashLadha
Copy link
Contributor

Will check today, and close on this.

@yashLadha
Copy link
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.

@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
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants