tools: fix inspector_protocol compiler warnings #39725

richardlau · 2021-08-10T16:54:36Z

This is an attempt to see if cherry-picking ffb34b6 (reverted by #39694) fixes the currently broken coverage workflow.

https://github.com/nodejs/node/actions/workflows/coverage-linux.yml?query=

/home/runner/work/node/node/out/Release/obj/gen/src/node/inspector/protocol/Protocol.cpp: In member function ‘void node::inspector::protocol::cbor::CBORTokenizer::ReadNextToken(bool)’:
/home/runner/work/node/node/out/Release/obj/gen/src/node/inspector/protocol/Protocol.cpp:2567:66: error: comparison of integer expressions of different signedness: ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
 2567 |           if (!bytes_read || std::numeric_limits<int32_t>::max() <
      |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2568 |                                  token_start_internal_value_) {
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~      
/home/runner/work/node/node/out/Release/obj/gen/src/node/inspector/protocol/Protocol.cpp:2585:58: error: comparison of integer expressions of different signedness: ‘uint64_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
 2585 |           if (!bytes_read || token_start_internal_value_ >
      |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2586 |                                  std::numeric_limits<int32_t>::max()) {
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [libnode.target.mk:410: /home/runner/work/node/node/out/Release/obj.target/libnode/gen/src/node/inspector/protocol/Protocol.o] Error 1

The second commit reenables coverage (the workflow that appears to be broken) for tools as that's where the inspector protocol files are (I forget the reason but it was moved there a long time ago) and the workflow wasn't run on #39694.

Original commit message: tools: fix compiler warning in inspector_protocol error: comparison of integer expressions of different signedness: ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] 2562 | if (!success || std::numeric_limits<int32_t>::max() < | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ 2563 | token_start_internal_value_) { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ PR-URL: nodejs#37573 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

richardlau · 2021-08-10T16:58:30Z

FWIW I've not been able to reproduce the warning/failure locally and I'm not sure why test-linux isn't also broken as it also uses --error-on-warn

node/.github/workflows/test-linux.yml

Line 31 in f8ed755

run: make build-ci -j2 V=1 CONFIG_FLAGS="--error-on-warn"

. (And this is not a draft so that the actions actually run.)

Trott · 2021-08-10T17:11:07Z

All the recent updates to inspector_protocol have been one-commit-at-a-time so bisecting should find the hopefully-narrow change causing issue rather than a big blobby update that needs to be reverted. Hopefully that's a virtue. (I can see the appeal of "just revert the whole big blobby update and move on".)

richardlau · 2021-08-10T17:14:55Z

All the recent updates to inspector_protocol have been one-commit-at-a-time so bisecting should find the hopefully-narrow change causing issue rather than a big blobby update that needs to be reverted. Hopefully that's a virtue. (I can see the appeal of "just revert the whole big blobby update and move on".)

Not sure I follow -- this PR isn't reverting anything (other than reverting the revert). Looks like coverage still fails so more investigation needed.

Trott · 2021-08-10T17:19:14Z

All the recent updates to inspector_protocol have been one-commit-at-a-time so bisecting should find the hopefully-narrow change causing issue rather than a big blobby update that needs to be reverted. Hopefully that's a virtue. (I can see the appeal of "just revert the whole big blobby update and move on".)

Not sure I follow -- this PR isn't reverting anything (other than reverting the revert). Looks like coverage still fails so more investigation needed.

The action starting failing after a series of 5 commits (and the 6th revert commit mentioned here) landed yesterday that update tools/inspector_protocol. One of those 6 commits probably broke this. (Either that or it was a coincidence of timing.) I'm mentioning that I've been updating tools/inspector_protocol one commit at a time specifically so we can find problems like this, as opposed to doing 50 commits all at once.

I guess I'm (weakly) defending my many-small-commits approach on this. :-D

Trott · 2021-08-10T17:23:38Z

(By the way, I'm kicking off some actions in my local repo to hopefully identify the commit that broke things.)

The inspector protocol currently lives in `tools`.

richardlau · 2021-08-10T19:41:56Z

I patched up another comparison, which was changed by c3df329. https://github.com/nodejs/node/pull/39725/checks?check_run_id=3293574323 has passed 🎉 .

Trott · 2021-08-10T22:10:02Z

I patched up another comparison, which was changed by c3df329. https://github.com/nodejs/node/pull/39725/checks?check_run_id=3293574323 has passed 🎉 .

I wonder if whatever that patching up fixes was eventually fixed upstream. If not, would you be up for submitting a patch there?

richardlau · 2021-08-10T22:41:28Z

I patched up another comparison, which was changed by c3df329. https://github.com/nodejs/node/pull/39725/checks?check_run_id=3293574323 has passed 🎉 .

I wonder if whatever that patching up fixes was eventually fixed upstream. If not, would you be up for submitting a patch there?

@RaisinTen do you want to add the extra change to https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/3077907?

Trott · 2021-08-10T23:39:42Z

.github/workflows/coverage-linux.yml

@@ -8,7 +8,6 @@ on:
      - 'benchmark/**'
      - 'deps/**'
      - 'doc/**'
-      - 'tools/**'


Ah, that's why this was missed in the PR!

Trott · 2021-08-10T23:44:18Z

.github/workflows/coverage-linux.yml

@@ -8,7 +8,6 @@ on:
      - 'benchmark/**'
      - 'deps/**'
      - 'doc/**'
-      - 'tools/**'


Have never used it myself, but based on https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#example-using-positive-and-negative-patterns-1, perhaps this would be better?

Suggested change

- 'tools/**'

paths:

- '!**.md'

- '!benchmark/**'

- '!deps/**'

- '!doc/**'

- '!tools/**'

- 'tools/inspector_protocol/**'

Whoops, that was supposed to be a suggestion to replace all of lines 6-11, not just line 11.

Maybe do this separately? I'm not sure without testing, for example, if the suggestion would include stuff under src/**, lib/**, etc. and as the GitHub docs mention the ordering matters.

Trott · 2021-08-10T23:45:30Z

.github/workflows/coverage-linux.yml

@@ -18,7 +17,6 @@ on:
      - 'benchmark/**'
      - 'deps/**'
      - 'doc/**'
-      - 'tools/**'


Same here as prior suggestion to use paths: instead of paths-ignore and use ! to negate each path, and then explicitly add tools/inspector_protocol/**.

Trott

LGTM to get the job fixed with or without my suggestions.

Trott · 2021-08-10T23:46:26Z

Thanks for finding/fixing this!

ghost

#39725 #38478

nodejs-github-bot · 2021-08-11T16:57:23Z

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

nodejs-github-bot · 2021-08-11T20:23:44Z

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

Original commit message: tools: fix compiler warning in inspector_protocol error: comparison of integer expressions of different signedness: ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] 2562 | if (!success || std::numeric_limits<int32_t>::max() < | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ 2563 | token_start_internal_value_) { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ PR-URL: #37573 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The inspector protocol currently lives in `tools`. PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2021-08-12T21:46:14Z

Landed in 13029cf...3b1e6f2

RaisinTen · 2021-08-14T09:11:45Z

I patched up another comparison, which was changed by c3df329. https://github.com/nodejs/node/pull/39725/checks?check_run_id=3293574323 has passed 🎉 .

I wonder if whatever that patching up fixes was eventually fixed upstream. If not, would you be up for submitting a patch there?

@RaisinTen do you want to add the extra change to https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/3077907?

@richardlau Thank you for the suggestion, added! :)

Original commit message: tools: fix compiler warning in inspector_protocol error: comparison of integer expressions of different signedness: ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] 2562 | if (!success || std::numeric_limits<int32_t>::max() < | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ 2563 | token_start_internal_value_) { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ PR-URL: #37573 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The inspector protocol currently lives in `tools`. PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: tools: fix compiler warning in inspector_protocol error: comparison of integer expressions of different signedness: ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] 2562 | if (!success || std::numeric_limits<int32_t>::max() < | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ 2563 | token_start_internal_value_) { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ PR-URL: #37573 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Aug 10, 2021

richardlau force-pushed the inspector branch from 5f1b7e7 to 2acaade Compare August 10, 2021 18:05

richardlau added 2 commits August 10, 2021 14:06

tools: fix more build warnings in inspector_protocol

e79973c

build: run coverage for inspector protocol changes

44104c6

The inspector protocol currently lives in `tools`.

BethGriggs mentioned this pull request Aug 10, 2021

deps: update c-ares to 1.17.2 #39724

Closed

richardlau force-pushed the inspector branch from 2acaade to 44104c6 Compare August 10, 2021 18:08

oluan mentioned this pull request Aug 10, 2021

dns: fix uv_timer_start usage #39723

Open

Trott reviewed Aug 10, 2021

View reviewed changes

Trott approved these changes Aug 10, 2021

View reviewed changes

ghost approved these changes Aug 11, 2021

View reviewed changes

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2021

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2021

richardlau changed the title ~~tools: cherry-pick ffb34b6d5dbf0~~ tools: fix inspector_protocol compiler warnings Aug 12, 2021

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

targos mentioned this pull request Aug 12, 2021

request-ci fails without error when CI is locked down #39746

Open

jasnell approved these changes Aug 12, 2021

View reviewed changes

jasnell pushed a commit that referenced this pull request Aug 12, 2021

tools: fix more build warnings in inspector_protocol

8c95410

PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell pushed a commit that referenced this pull request Aug 12, 2021

build: run coverage for inspector protocol changes

3b1e6f2

The inspector protocol currently lives in `tools`. PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell closed this Aug 12, 2021

richardlau deleted the inspector branch August 12, 2021 22:20

danielleadams pushed a commit that referenced this pull request Aug 16, 2021

tools: fix more build warnings in inspector_protocol

52a172f

PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams pushed a commit that referenced this pull request Aug 16, 2021

build: run coverage for inspector protocol changes

097d898

The inspector protocol currently lives in `tools`. PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams mentioned this pull request Aug 16, 2021

v16.7.0 release proposal #39782

Merged

targos added the dont-land-on-v14.x label Sep 4, 2021

targos pushed a commit that referenced this pull request Sep 4, 2021

tools: fix more build warnings in inspector_protocol

a338c0e

PR-URL: #39725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Sep 4, 2021

v14.18.0 release proposal #39990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: fix inspector_protocol compiler warnings #39725

tools: fix inspector_protocol compiler warnings #39725

richardlau commented Aug 10, 2021

richardlau commented Aug 10, 2021

Trott commented Aug 10, 2021 •

edited

Loading

richardlau commented Aug 10, 2021

Trott commented Aug 10, 2021

Trott commented Aug 10, 2021

richardlau commented Aug 10, 2021

Trott commented Aug 10, 2021

richardlau commented Aug 10, 2021

Trott Aug 10, 2021

Trott Aug 10, 2021

Trott Aug 10, 2021

richardlau Aug 11, 2021

Trott Aug 10, 2021

Trott left a comment

Trott commented Aug 10, 2021

ghost left a comment

nodejs-github-bot commented Aug 11, 2021

nodejs-github-bot commented Aug 11, 2021

jasnell commented Aug 12, 2021

RaisinTen commented Aug 14, 2021

-      - 'tools/**'
+    paths:
+      - '!**.md'
+      - '!benchmark/**'
+      - '!deps/**'
+      - '!doc/**'
+      - '!tools/**'
+      - 'tools/inspector_protocol/**'

tools: fix inspector_protocol compiler warnings #39725

tools: fix inspector_protocol compiler warnings #39725

Conversation

richardlau commented Aug 10, 2021

richardlau commented Aug 10, 2021

Trott commented Aug 10, 2021 • edited Loading

richardlau commented Aug 10, 2021

Trott commented Aug 10, 2021

Trott commented Aug 10, 2021

richardlau commented Aug 10, 2021

Trott commented Aug 10, 2021

richardlau commented Aug 10, 2021

Trott Aug 10, 2021

Choose a reason for hiding this comment

Trott Aug 10, 2021

Choose a reason for hiding this comment

Trott Aug 10, 2021

Choose a reason for hiding this comment

richardlau Aug 11, 2021

Choose a reason for hiding this comment

Trott Aug 10, 2021

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

Trott commented Aug 10, 2021

ghost left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 11, 2021

nodejs-github-bot commented Aug 11, 2021

jasnell commented Aug 12, 2021

RaisinTen commented Aug 14, 2021

Trott commented Aug 10, 2021 •

edited

Loading