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.

Sign up for GitHub

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

Jump to bottom

Enhancements to --build-v8-with-gn #58

Closed
wants to merge 2 commits into from
Closed

Enhancements to --build-v8-with-gn #58

wants to merge 2 commits into from

Conversation

agrieve
Copy link

@agrieve agrieve commented Apr 17, 2018

  • Split "gn gen" and "ninja" invocations into two separate actions
  • Make the ninja action use console pool so that progress is shown
  • Add --build-v8-with-gn-use-goma for faster builds (for Googlers)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@agrieve
Enhancements to --build-v8-with-gn
984e470 
* Split "gn gen" and "ninja" invocations into two separate actions
* Make the ninja action use console pool so that progress is shown
* Add --build-v8-with-gn-use-goma for faster builds (for Googlers)
@agrieve agrieve mentioned this pull request Apr 17, 2018
Closed
hashseed
hashseed reviewed Apr 17, 2018
View reviewed changes
configure
@@ -557,6 +557,12 @@ parser.add_option('--build-v8-with-gn',
dest='build_v8_with_gn',
default=False,
help='build V8 using GN instead of gyp')
parser.add_option('--build-v8-with-gn-use-goma',
Copy link
Member

Choose a reason for hiding this comment

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

Could this also include --build-v8-with-gn? Otherwise both flags would have to be passed.

Copy link
Author

Choose a reason for hiding this comment

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

sgtm. Done.

hashseed
hashseed reviewed Apr 17, 2018
View reviewed changes
deps/v8/tools/node/build_gn.py
@@ -46,7 +46,6 @@ def FindGn(options):
return os.path.join(options.v8_path, "buildtools", os_path, "gn")
Copy link
Member

Choose a reason for hiding this comment

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

This file is directly copied over from V8 upstream (e.g. using v8/tools/update_node.py <v8-path> <node-path>), so changes to this should land in V8 upstream.

Copy link
Author

Choose a reason for hiding this comment

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

Copied it to: https://chromium-review.googlesource.com/c/v8/v8/+/1016582

@hashseed
Copy link
Member

I somehow run into exceptions in build_gn.py when building (i.e. when called with --build) both when I build with ninja and when I build with make.

[2018-04-18 14:29:14] yangguo@yangguo2:~/node$ ninja -C out/Release/ v8_monolith -j100
ninja: Entering directory `out/Release/'
[0/2] ACTION v8_monolith: build_with_gn_35e8d5ae236c975b687639dbe52d07d7
Using depot tools in ../_depot_tools
Traceback (most recent call last):
  File "../tools/node/build_gn.py", line 111, in <module>
    Build(options)
  File "../tools/node/build_gn.py", line 78, in Build
    subprocess.check_call(args, cwd=options.v8_path)
  File "/usr/lib/python2.7/subprocess.py", line 181, in check_call
    retcode = call(*popenargs, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 168, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 390, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1024, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
FAILED: obj/deps/v8/gypfiles/v8_monolith.gen/gn/obj/libv8_monolith.a ../../deps/v8/gypfiles/does-not-exist 
cd ../../deps/v8/gypfiles; python ../tools/node/build_gn.py --build_path ../../../out/Release/obj/deps/v8/gypfiles/v8_monolith.gen/gn --v8_path ../ --build
ninja: build stopped: subcommand failed.

agrieve
agrieve commented Apr 18, 2018
View reviewed changes
Copy link
Author

@agrieve agrieve left a comment

Choose a reason for hiding this comment

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

Confusing with two PRs. Going to just close this one but keep the two commits separate in #60.

@agrieve agrieve closed this Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashseed hashseed hashseed left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@agrieve @hashseed