Revert "build: enabling pgo at configure"

[addaleax]

This reverts commit 9be1555.

This commit broke native addon compilation by adding
variables to common.gypi that are not available
to builds outside of Node.js core.

Checklist

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

[nodejs-github-bot]

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/840/pipeline

[addaleax]

Fyi, reviewers of the reverted commit: @octaviansoldea@jasnell@richardlau@lundibundi

[addaleax]

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

Please 👍 this comment to approve fast-tracking.

[mscdex]

Is there a way we can add a simple addon test for this?

[addaleax]

@mscdex Just about any addon will experience this issue – I think CITGM actually might/should have caught this? It wasn’t run on the original PR.

I’m not sure why our own addon tests didn’t catch this, but I assume it’s because we build this with our tree-wide config? We’ve had similar issues with e.g. headers in the past…

[richardlau]

Agreed with the comments elsewhere that the additions should in hindsight been made to node.gyp instead of common.gypi.

Having said that, I'll try to look tomorrow but I was under the impression that node-gyp defaulted the config.gypi that gets generated for the addon to process.config from the node running node-gyp and should therefore pick up the new variable unless you are cross compiling from a different version of Node.js.

https://github.com/nodejs/node-gyp/blob/9a404d6d36dcf9c7be2ae9963019c4d89bbb9155/lib/configure.js#L116-L118

[refack]

Agreed with the comments elsewhere that the additions should in hindsight been made to node.gyp instead of common.gypi.

IMHO it should have be in common.gypi so that all the deps would be built with pgo, it just needed a default in the head of the file.

I'm too am intrigued why it passed our CI, but failed in the wild.

[richardlau]

I'm too am intrigued why it passed our CI, but failed in the wild.

As previously mentioned, our regular CI builds addons with node-gyp with --nodedir to point to the Node.js source tree, which is not the same as using the headers tarball which is the more common case elsewhere (not all headers are in the tarball, for example, and those that are are in a different directory structure).

[refack]

This commit broke native addon compilation by adding

@addaleax Could you test it again? Make sure you don't have a global npm_config_nodedir?
I'm able to build native-addons with the latest nightly. node-gyp generates the following config.gypi:

# Do not edit. File was generated by node-gyp's "configure" step{... "variables":{"asan": 0, "build_v8_with_gn": "false", "coverage": "false", "debug_nghttp2": "false", "enable_lto": "false", "enable_pgo_generate": "false", "enable_pgo_use": "false", "force_dynamic_crt": 0, ... }

[addaleax]

@refack Right, that works … so is there something to do here, or was this just caused by stale configs?

[refack]

Right, that works … so is there something to do here, or was this just caused by stale configs?

I'm for adding:

 common.gypi | 2 ++ 1 file changed, 1 insertions(+) diff --git a/common.gypi b/common.gypi index 1fda1dde79..a6138b93c3 100644 --- a/common.gypi+++ b/common.gypi@@ -9,6 +9,8 @@ 'library%': 'static_library', # allow override to 'shared_library' for DLL/.so builds 'component%': 'static_library', # NB. these names match with what V8 expects 'msvs_multi_core_compile': '0', # we do enable multicore compiles, but not using the V8 way + 'enable_pgo_generate%': '0',+ 'enable_pgo_use%': '0', 'python%': 'python', 'node_shared%': 'false',

There is a subtle bug here. We do use node in two separate roles here: (1) as the build scaffolding tool (2) as a lib. If you want to build an addon for node11 using node8, node-gyp will probably use a wrong config.gypi (from node8's process.config)

[addaleax]

@refack Okay, sounds good to me. Do you want to close this PR and open a new one with that?

[addaleax]

Closing given the above discussion (and gentle ping @refack)