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

tools: fix inspector_protocol compiler warnings #39725

Closed
wants to merge 3 commits into from

Conversation

richardlau
Copy link
Member

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=

image

/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>
@nodejs-github-bot 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
Copy link
Member Author

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

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
Copy link
Member

Trott commented Aug 10, 2021

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
Copy link
Member Author

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
Copy link
Member

Trott commented Aug 10, 2021

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
Copy link
Member

Trott commented Aug 10, 2021

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

@richardlau
Copy link
Member Author

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
Copy link
Member

Trott commented Aug 10, 2021

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
Copy link
Member Author

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?

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Aug 10, 2021

Thanks for finding/fixing this!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2021
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau changed the title tools: cherry-pick ffb34b6d5dbf0 tools: fix inspector_protocol compiler warnings Aug 12, 2021
@richardlau 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
jasnell pushed a commit that referenced this pull request Aug 12, 2021
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>
jasnell pushed a commit that referenced this pull request Aug 12, 2021
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
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
Copy link
Member

jasnell commented Aug 12, 2021

Landed in 13029cf...3b1e6f2

@jasnell jasnell closed this Aug 12, 2021
@richardlau richardlau deleted the inspector branch August 12, 2021 22:20
@RaisinTen
Copy link
Contributor

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! :)

danielleadams pushed a commit that referenced this pull request Aug 16, 2021
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>
danielleadams pushed a commit that referenced this pull request Aug 16, 2021
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
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>
targos pushed a commit that referenced this pull request Sep 4, 2021
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>
targos pushed a commit that referenced this pull request Sep 4, 2021
PR-URL: #39725
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants