build: turn warnings into errors for node sources

[danbev]

This pr attempts to turn compilation warnings into errors for node's source code.

The motivation for this is to enable these errors to be reported by CI runs on pull requests and be able to fix them before they land.

Checklist

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

[nodejs-github-bot]

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

[mscdex]

Will this cause issues for GCC < 8 and warnings from v8.h?

[addaleax]

The motivation for this is to enable these errors to be reported by CI runs on pull requests and be able to fix them before they land.

Can we enable it only for CI then, in some way? I’m worried that we won’t know which environments consumers of our source code compile in and that we won’t know what warnings they receive, particularly warnings from future compiler versions that we aren’t yet aware of.

[danbev]

Will this cause issues for GCC < 8 and warnings from v8.h?

I'm not sure about GCC < 8 and for warnings from v8.h they would generate errors instead of warnings.

[danbev]

I’m worried that we won’t know which environments consumers of our source code compile in and that we won’t know what warnings they receive, particularly warnings from future compiler versions that we aren’t yet aware of.

That is a good point.

I'm not convinced this is actually worth the effort but I was asked to take a look at doing this and this is what I came up with. I don't see warnings as a big issue and they get fixed pretty quickly as it is.

[sam-github]

I think it could fix a lot of churn if code with warnings (in src under our control!) failed CI, since we CI with known compiler levels, it should be stable-ish. I guess it won't stop all warnings... I'm using clang 11 locally, and its pretty strict.

For local builds, many projects have a ./configure --dev-mode or something of the sort that enables dep tracking, -Werror, etc, but the configure doesn't default to that, so casual devs and build consumers won't see it (but they will if they PR).

[danbev]

I've added a commit with a suggestion for this.
A new configuration flag:

 ./configure --help | grep -A1 error-on-warn
  --error-on-warn       Turn compiler warnings into errors for node core
                        sources.

This can be tested by adding an unused variable to some code in node core:

$ ./configure
$ make -j8
../src/node.cc: In member function ‘int node::Environment::InitializeInspector(std::unique_ptr<node::inspector::ParentInspectorHandle>)’:
../src/node.cc:208:7: warning: unused variable ‘x’ [-Wunused-variable]
  208 |   int x = 10;
      |  

And using the new configuration option:

$ ./configure --error-on-warn
$ make -j8
../src/node.cc: In member function ‘int node::Environment::InitializeInspector(std::unique_ptr<node::inspector::ParentInspectorHandle>)’:
../src/node.cc:208:7: error: unused variable ‘x’ [-Werror=unused-variable]
  208 |   int x = 10;
      |       ^
cc1plus: all warnings being treated as errors

Would something like this work if we add it to the CI jobs?

[richardlau]

I've added a commit with a suggestion for this.
A new configuration flag:

 ./configure --help | grep -A1 error-on-warn
  --error-on-warn       Turn compiler warnings into errors for node core
                        sources.

Would something like this work if we add it to the CI jobs?

Sounds reasonable to me.

[nodejs-github-bot]

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

[richardlau]

LGTM. For enabling on the CI by default we will need to edit the individual jobs. My plan would be:

1. Land this PR
2. Run a platform's node-test-commit-* job (e.g. https://ci.nodejs.org/job/node-test-commit-linux/) with the CONFIG_FLAGS parameter set to --error-on-warn. Check that the job succeeds (they won't at the moment until at least src: fix compiler warnings in node_http2.cc #33014 lands).
3. If the job succeeds, edit the job to add --error-on-warn as a default parameter, otherwise if it fails raise an issue/PR for fixing the warning.

[danbev]

I'd like to land this but this is the first time I've seen a failure for the build-tarball job. Has anyone else see this before? (I'm able to run ./configure && make -j8 tar locally without any failure)

[jasnell]

Would be great to have this in the Windows vcbuild.bat also.

[danbev]

Yeah, I agree. The problem for me is that I don't have a windows testing environment anymore. At least if this got merged we could enable this for pull requests.

[mmarchini]

Can be done on a follow up, but I think it makes sense to set this flag on Actions :)

[danbev]

Landed in 2e441e1, and a7ae7aa.

[richardlau]

I've cherry-picked a7ae7aa over to v13.x-staging and enabled the flag (after testing) for the UBI 8.1 Linux containered job (which only runs on Node.js 13+). I'd like to avoid, if possible, extra checks in the jobs to detect if the --error-on-warn flag exists, so other jobs will have to wait until we land the flag in 12.x and 10.x.