Build and gyp fixes #23156

refack · 2018-09-29T02:21:01Z

This is a spin off from work I did for integrating V8 7.1 (#22920), but this part works with master to reduce cases of unneeded rebuild make && make will do a lot of work twice.

This also fixes a few small bugs.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-09-29T02:21:04Z

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

refack · 2018-09-29T02:21:23Z

/CC @nodejs/build-files @nodejs/gyp @nodejs/python

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

richardlau

Are the gyp changes being upstreamed/applicable to node-gyp?

richardlau · 2018-09-29T02:51:56Z

common.gypi

@@ -367,7 +367,6 @@
        'ldflags': [ '-pthread' ],
      }],
      [ 'OS in "linux freebsd openbsd solaris android aix cloudabi"', {
-        'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', ],


Some of these are also set for mac on L466-L471.

richardlau · 2018-09-29T02:56:33Z

node.gypi

@@ -24,6 +24,7 @@
    },
    'force_load%': '<(force_load)',
  },
+  'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', ],


Does this need to be in an OS conditional as it was in common.gypi?

AFAICT GYP knows what to do, so no need to exclude win and mac.
I'll investigate the CI output

I'll investigate the CI output

On freebsd (which uses clang)

ccache c++ -o /out/Release/obj.target/node_lib/src/callback_scope.o ... -m64 -Wall -Wextra -Wno-unused-parameter

On Mac (which is clang bu via XCODE):

/usr/local/bin/ccache c++ -o out/Release/obj.target/node_lib/src/callback_scope.o ../src/callback_scope.cc ... -Wall -Wendif-labels -W -Wno-unused-parameter

(only the old set, this run was before I moved it)

On Windows
https://gist.github.com/refack/aa6d6bac65bcd0ab4187aa86bfc27fce
No mention of any of the cflags

joyeecheung

It's probably a good idea to add a few comments in the gypfiles mentioning they are actually maintained by us as we keep adding our own patches to the files...

joyeecheung · 2018-09-29T09:23:37Z

node.gyp

      'dependencies': [
-        'node_js2c#host',
+        'mkssldef',


Is there a reason this is made a dependency? It looks like an action in the same spirit of js2c

Short answer is, no reason not to.
The situation that we got these two changes was done in two independant steps (so I didn't notice that it looks like a switch):

If mkssldef in not there, the build breaks on windows if building with more than 2 threads.

If node_js2c#host is a target and not an action, it leads to unnecessary reruns (then compilation) on make based systems.

refack · 2018-09-29T20:13:44Z

I moved some more v8.gyp refactoring from #22920
~~https://ci.nodejs.org/job/node-test-pull-request/17509/~~
https://ci.nodejs.org/job/node-test-pull-request/17510/

refack · 2018-09-29T21:14:45Z

It's probably a good idea to add a few comments in the gypfiles mentioning they are actually maintained by us as we keep adding our own patches to the files...

I'm going to move them to tools/v8gypfiles, I have a WIP, I just don't want to conflict my other PRs

addaleax

Since all of these seem more or less independent of each other, maybe it’s best to split them into one PR each?

common.gypi

node.gyp

Makefile

addaleax · 2018-09-30T15:50:59Z

.gitignore

@@ -4,6 +4,7 @@
 !test/fixtures/**/.*
 !tools/node_modules/**/.*
 !tools/doc/node_modules/**/.*
+!deps/npm/node_modules


This should probably not go in the dotfiles section?

.gitignore

addaleax · 2018-09-30T15:51:29Z

src/inspector/node_inspector.gypi

+      ],
+    },
+  ],
+}


missing newline

addaleax · 2018-09-30T15:52:43Z

node.gyp

-            '<(SHARED_INTERMEDIATE_DIR)',
-            '<(SHARED_INTERMEDIATE_DIR)/src', # for inspector
-          ],
+          'includes' : [ 'src/inspector/node_inspector.gypi' ],


I think this would be more consistent if we also had e.g. the main sources list for src/ in src/node.gypi or similar…

That's high on my backlog.
Might also have a compile only target for /src/ similar to v8_base, then do the linking in node_lib. So you can run compile with no dependencies by running make -C out node_base

addaleax · 2018-09-30T15:53:18Z

common.gypi

@@ -367,7 +367,6 @@
        'ldflags': [ '-pthread' ],
      }],
      [ 'OS in "linux freebsd openbsd solaris android aix cloudabi"', {
-        'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', ],


Does this disable the warnings by default for addons? That would be pretty bad…

It doesn't exactly disable, it "un-enables", but yes it changes default behaviour.
This does bring up the issue of common.gypi not exactly being the right fix for addons (nodejs/node-gyp#1118)

Authors could set their own warning level in the addons binding.gyp.

If you think this should not change for addons, I could try to figure out another mechanism.

Are you okay with leaving that out of this PR? We can revisit it once something has happened along the lines of the PR you linked…

refack · 2018-09-30T17:09:07Z

Since all of these seem more or less independent of each other, maybe it’s best to split them into one PR each?

They were all developed together as a basis for #22920, but I figured they could benefit master independently.
Maybe the .gitignore is the oddball here, but IMHO having them all as a cohesive "narrative" makes reviewing easier.

addaleax · 2018-09-30T17:13:42Z

Maybe the .gitignore is the oddball here, but IMHO having them all as a cohesive "narrative" makes reviewing easier.

A number of small PRs are usually a lot easier to review than a single PR with an impressive +679/−777 diff, especially when the commits in it are largely independent.

refack · 2018-09-30T17:16:59Z

an impressive +679/−777 diff

Ohh it grew that big because of the moves.
I'll break this up.

refack · 2018-09-30T17:47:25Z

Took out the .gitignore (#23180) and V8 changes (#23182) PTAL

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

richardlau

Still LGTM.

refack · 2018-10-01T17:54:54Z

Resume CI: https://ci.nodejs.org/job/node-test-commit/21945/

refack · 2018-10-01T18:30:07Z

Single windows fail (not ok 582 sequential/test-gc-http-client) is #22336

Another resume: https://ci.nodejs.org/job/node-test-commit/21946/

thefourtheye · 2018-10-02T05:51:55Z

node.gyp

          ],
        },
      ],
-    }, # end node_js2c
+    }, # node_lib_target_name
+    {     # generate ETW header and resource files


Tiny nit: This comment might look better in the previous line.

Moving it was a cosmetic decision:

I could move it one line lower to the top of the target

thefourtheye · 2018-10-02T05:54:15Z

tools/gyp/pylib/gyp/generator/make.py

      self.WriteLn('%s: %s' % ('.INTERMEDIATE', intermediate))
-      self.WriteLn('%s: %s%s' %
-                   (intermediate, ' '.join(inputs), force_append))


force_append is actually printed in all the other control paths of this if..else ladder. Why remove it only here?

This if clause if for an action with multiple outputs (the only case that uses an intermediate target). This structure causes the action to always rerun.

AFAICT it's because the XXXXXX.intermediate file doesn't have a build rule, it is always touched which causes the targets that depend of it to be re made.

Prerequisite 'FORCE_DO_CMD' of target '19ae1b9357ee828c575655f479c768b510a5ca14.intermediate' does not exist. Must remake target '19ae1b9357ee828c575655f479c768b510a5ca14.intermediate'. make: Entering directory '/mnt/d/code/node/out' touch 19ae1b9357ee828c575655f479c768b510a5ca14.intermediate Prerequisite '19ae1b9357ee828c575655f479c768b510a5ca14.intermediate' is newer than target '/mnt/d/code/node/out/Debug/obj/gen/src/inspector/protocol/Forward.h'. Must remake target '/mnt/d/code/node/out/Debug/obj/gen/src/inspector/protocol/Forward.h'. Successfully remade target file '/mnt/d/code/node/out/Debug/obj/gen/src/inspector/protocol/Forward.h'. Prerequisite '19ae1b9357ee828c575655f479c768b510a5ca14.intermediate' is newer than target '/mnt/d/code/node/out/Debug/obj/gen/src/inspector/protocol/Protocol.cpp'. Must remake target '/mnt/d/code/node/out/Debug/obj/gen/src/inspector/protocol/Protocol.cpp'. Successfully remade target file '/mnt/d/code/node/out/Debug/obj/gen/src/inspector/protocol/Protocol.cpp'.

I tested the generated makefile, and it seems to still work as expected after this change. (touching a dependency of the action, touching an output, and deleting a generated output).

Anyway I'll document this.

refack · 2018-10-02T21:56:33Z

Landed in
6dd4a07
375e16e
731a72f
5d8373a

Don't add `force_append` (FORCE_DO_CMD) to the intermediate sentinal. Adding it makes the action run alway, even when there are no changes. (refack): AFAICT because `*.intermediate` files don't have build rules. PR-URL: #23156 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Puts the compilation target upfront for easy reading. PR-URL: #23156 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: #23156 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Run `node_js2c` and `mkssldef` as actions and not as targets makes sure they are run only once, just before processing the rest of `node_lib`. This helps `make` based dependency change detection be more accurate. Add comments with tagrget names for readability. Use `process_outputs_as_sources` for automatic inclution of outputs. PR-URL: #23156 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

* Make inspector.gypi and v8_external_snapshot.gypi includible targets. * Make `v8_dump_build_config` an action * Better separate `js2c` and `natives_blob` * process action outputs as sources * trigger v8.gyp:postmortem-metadata from v8.gyp PR-URL: nodejs#23182 Refs: nodejs#23156 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

* explicitly unignore files that we track. The following are not created anymore (only as subdirs of v8/gypfiles) /deps/v8/src/debug/obj deps/v8/src/Debug/ deps/v8/src/Release/ deps/v8/src/inspector/Debug/ deps/v8/src/inspector/Release/ PR-URL: nodejs#23180 Refs: nodejs#23156 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Make inspector.gypi and v8_external_snapshot.gypi includible targets. * Make `v8_dump_build_config` an action * Better separate `js2c` and `natives_blob` * process action outputs as sources * trigger v8.gyp:postmortem-metadata from v8.gyp PR-URL: #23182 Refs: #23156 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

* explicitly unignore files that we track. The following are not created anymore (only as subdirs of v8/gypfiles) /deps/v8/src/debug/obj deps/v8/src/Debug/ deps/v8/src/Release/ deps/v8/src/inspector/Debug/ deps/v8/src/inspector/Release/ PR-URL: #23180 Refs: #23156 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Make inspector.gypi and v8_external_snapshot.gypi includible targets. * Make `v8_dump_build_config` an action * Better separate `js2c` and `natives_blob` * process action outputs as sources * trigger v8.gyp:postmortem-metadata from v8.gyp PR-URL: #23182 Refs: #23156 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

* explicitly unignore files that we track. The following are not created anymore (only as subdirs of v8/gypfiles) /deps/v8/src/debug/obj deps/v8/src/Debug/ deps/v8/src/Release/ deps/v8/src/inspector/Debug/ deps/v8/src/inspector/Release/ PR-URL: #23180 Refs: #23156 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 29, 2018

refack added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Sep 29, 2018

refack requested review from richardlau, targos and joyeecheung September 29, 2018 02:21

refack self-assigned this Sep 29, 2018

richardlau approved these changes Sep 29, 2018

View reviewed changes

richardlau reviewed Sep 29, 2018

View reviewed changes

joyeecheung reviewed Sep 29, 2018

View reviewed changes

refack force-pushed the build-and-gyp-fixes branch from 8bb3463 to a12c082 Compare September 29, 2018 20:01

refack force-pushed the build-and-gyp-fixes branch from 35cb103 to f0725e5 Compare September 29, 2018 23:02

addaleax reviewed Sep 30, 2018

View reviewed changes

This was referenced Sep 30, 2018

build: cleanup in .gitignore #23180

Merged

build,deps: refactor and fix v8.gyp #23182

Closed

refack force-pushed the build-and-gyp-fixes branch from 4c766d6 to 9ee211e Compare September 30, 2018 17:46

refack removed the request for review from targos September 30, 2018 17:47

richardlau approved these changes Sep 30, 2018

View reviewed changes

joyeecheung approved these changes Sep 30, 2018

View reviewed changes

thefourtheye reviewed Oct 2, 2018

View reviewed changes

refack force-pushed the build-and-gyp-fixes branch from e603a9b to 5d8373a Compare October 2, 2018 21:54

refack merged commit 5d8373a into nodejs:master Oct 2, 2018

refack deleted the build-and-gyp-fixes branch October 2, 2018 21:56

targos pushed a commit that referenced this pull request Oct 3, 2018

build: encapsulate node/inspector gyp scafolding

aae0ece

PR-URL: #23156 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

refack mentioned this pull request Oct 3, 2018

Missing out/Debug/node on test-digitalocean-ubuntu1604_sharedlibs_container-x64-* nodejs/build#1516

Closed

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

refack removed their assignment Oct 12, 2018

refack mentioned this pull request Oct 19, 2018

build: prevent C++ linker from overwriting executable #22027

Closed

2 tasks

refack mentioned this pull request Oct 30, 2018

tools,gyp: don't force build actions with multiple outputs #23982

Closed

2 tasks

Build and gyp fixes #23156

Build and gyp fixes #23156

Conversation

refack commented Sep 29, 2018

Checklist

nodejs-github-bot commented Sep 29, 2018

refack commented Sep 29, 2018 • edited Loading

richardlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Sep 29, 2018 • edited Loading

refack commented Sep 29, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Sep 30, 2018

addaleax commented Sep 30, 2018

refack commented Sep 30, 2018

refack commented Sep 30, 2018 • edited Loading

richardlau left a comment

Choose a reason for hiding this comment

refack commented Oct 1, 2018

refack commented Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

refack Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 2, 2018

refack commented Sep 29, 2018 •

edited

Loading

refack commented Sep 29, 2018 •

edited

Loading

refack commented Sep 30, 2018 •

edited

Loading

refack commented Oct 1, 2018 •

edited

Loading

refack Oct 2, 2018 •

edited

Loading