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

Build and gyp fixes #23156

Merged
merged 4 commits into from
Oct 2, 2018
Merged

Build and gyp fixes #23156

merged 4 commits into from
Oct 2, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Sep 29, 2018

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

refack commented Sep 29, 2018

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

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

@refack 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 refack self-assigned this Sep 29, 2018
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved them

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

node.gyp Outdated
'dependencies': [
'node_js2c#host',
'mkssldef',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  1. If mkssldef in not there, the build breaks on windows if building with more than 2 threads.
  2. If node_js2c#host is a target and not an action, it leads to unnecessary reruns (then compilation) on make based systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved.

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

refack commented Sep 29, 2018

@refack
Copy link
Contributor Author

refack commented Sep 29, 2018

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

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

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
node.gyp Show resolved Hide resolved
Makefile Show resolved Hide resolved
.gitignore Outdated
@@ -4,6 +4,7 @@
!test/fixtures/**/.*
!tools/node_modules/**/.*
!tools/doc/node_modules/**/.*
!deps/npm/node_modules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably not go in the dotfiles section?

.gitignore Outdated Show resolved Hide resolved
],
},
],
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline

'<(SHARED_INTERMEDIATE_DIR)',
'<(SHARED_INTERMEDIATE_DIR)/src', # for inspector
],
'includes' : [ 'src/inspector/node_inspector.gypi' ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@refack
Copy link
Contributor Author

refack commented Sep 30, 2018

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

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

refack commented Sep 30, 2018

an impressive +679/−777 diff

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

@refack
Copy link
Contributor Author

refack commented Sep 30, 2018

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

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

@refack refack removed the request for review from targos September 30, 2018 17:47
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM.

@refack
Copy link
Contributor Author

refack commented Oct 1, 2018

@refack
Copy link
Contributor Author

refack commented Oct 1, 2018

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

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

node.gyp Outdated
],
},
],
}, # end node_js2c
}, # node_lib_target_name
{ # generate ETW header and resource files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@refack refack Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving it was a cosmetic decision:
image
I could move it one line lower to the top of the target

self.WriteLn('%s: %s' % ('.INTERMEDIATE', intermediate))
self.WriteLn('%s: %s%s' %
(intermediate, ' '.join(inputs), force_append))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 refack force-pushed the build-and-gyp-fixes branch from e603a9b to 5d8373a Compare October 2, 2018 21:54
@refack refack merged commit 5d8373a into nodejs:master Oct 2, 2018
@refack
Copy link
Contributor Author

refack commented Oct 2, 2018

Landed in
6dd4a07
375e16e
731a72f
5d8373a

@refack 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
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>
targos pushed a commit that referenced this pull request Oct 3, 2018
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>
targos pushed a commit that referenced this pull request Oct 3, 2018
PR-URL: #23156
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
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>
targos pushed a commit to targos/node that referenced this pull request Oct 4, 2018
* 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>
refack added a commit to refack/node that referenced this pull request Oct 4, 2018
* 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>
targos pushed a commit that referenced this pull request Oct 5, 2018
* 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>
targos pushed a commit that referenced this pull request Oct 5, 2018
* 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>
targos pushed a commit that referenced this pull request Oct 7, 2018
* 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>
@refack refack removed their assignment Oct 12, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 2018
* 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>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
* 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>
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. lib / src Issues and PRs related to general changes in the lib or src directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants