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: enabling pgo at configure" #22772

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 9, 2018

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

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.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Sep 9, 2018
@addaleax
Copy link
Member Author

addaleax commented Sep 9, 2018

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

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 9, 2018
@addaleax
Copy link
Member Author

addaleax commented Sep 9, 2018

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

Please 👍 this comment to approve fast-tracking.

@mscdex
Copy link
Contributor

mscdex commented Sep 9, 2018

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

@addaleax
Copy link
Member Author

addaleax commented Sep 9, 2018

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

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

refack commented Sep 9, 2018

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

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

refack commented Sep 9, 2018

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 addaleax removed the fast-track PRs that do not need to wait for 48 hours to land. label Sep 10, 2018
@addaleax
Copy link
Member Author

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

@refack
Copy link
Contributor

refack commented Sep 10, 2018

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

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

@addaleax
Copy link
Member Author

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

@addaleax addaleax closed this Sep 14, 2018
@addaleax addaleax deleted the revert-pgo branch September 14, 2018 16:56
@lundibundi lundibundi mentioned this pull request Sep 25, 2018
4 tasks
lundibundi added a commit to lundibundi/node that referenced this pull request Sep 26, 2018
addaleax pushed a commit that referenced this pull request Oct 4, 2018
Refs: #22772 (comment)

PR-URL: #23102
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 5, 2018
Refs: #22772 (comment)

PR-URL: #23102
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 7, 2018
Refs: #22772 (comment)

PR-URL: #23102
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Refs: #22772 (comment)

PR-URL: #23102
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.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
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