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

Revert "build: silence cpp lint by default" #26358

Merged
merged 1 commit into from
Mar 7, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Feb 28, 2019

This reverts commit 0373836.

PR effect is semver major, and removes an escape hatch.
This landed without proper review from @nodejs/build-files

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

@refack refack requested a review from BridgeAR February 28, 2019 18:01
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 28, 2019
@refack
Copy link
Contributor Author

refack commented Feb 28, 2019

IIUC the original intent was to change

node/Makefile

Lines 57 to 60 in 0373836

# Default to verbose builds.
# To do quiet/pretty builds, run `make V=` to set V to an empty string,
# or set the V environment variable to an empty string.
V ?= 1

@refack refack self-assigned this Feb 28, 2019
@refack refack added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 28, 2019
@BridgeAR
Copy link
Member

@refack the regular cpp linting output is super verbose. This does not seem to be a good default. And in what way is the behavior semver-major? Do we count the output of our makefile towards semver-major?

I guess ideally, we'd have an opt-in to get the verbose cpp linting output.

@BridgeAR BridgeAR removed the fast-track PRs that do not need to wait for 48 hours to land. label Mar 3, 2019
@danbev
Copy link
Contributor

danbev commented Mar 7, 2019

I would prefer to make lint-cpp quiet by default as it does not really provide any information except that what files it is processing. I understand that this is possible using the verbose flag but at least I personally don't use it very often.

I don't want to block this as there are already approvals so I'm going to remove myself from the reviews list.

@danbev danbev removed their request for review March 7, 2019 07:50
@joyeecheung
Copy link
Member

Why is the effect semver-major?

@refack
Copy link
Contributor Author

refack commented Mar 7, 2019

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

Why is the effect semver-major?

And in what way is the behavior semver-major?

  1. Output of the build process is semver-major for downstream embedders (and you CI scripts).
  2. In principal the Makefile should be able to output full logs for tracing & debugging. The reverted commit disabled any way of getting that output.
  3. I'd be +1 on changing

    node/Makefile

    Lines 57 to 60 in 0373836

    # Default to verbose builds.
    # To do quiet/pretty builds, run `make V=` to set V to an empty string,
    # or set the V environment variable to an empty string.
    V ?= 1

    In the meantime make V=0 should work just fine.

@refack
Copy link
Contributor Author

refack commented Mar 7, 2019

P.S. the lint-cpp target is timestamp aware. So only files changed should show up in the default output.

This reverts commit 0373836.

PR-URL: nodejs#26358
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@refack refack merged commit 91e1a04 into nodejs:master Mar 7, 2019
@refack refack deleted the revert-0373836 branch March 8, 2019 14:48
@refack refack removed their assignment Mar 11, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
This reverts commit 0373836.

PR-URL: nodejs#26358
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
This reverts commit 0373836.

PR-URL: #26358
Reviewed-By: Richard Lau <[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
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants