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

v8: include right headers in torque output #133

Closed
wants to merge 35 commits into from
Closed

v8: include right headers in torque output #133

wants to merge 35 commits into from

Conversation

bnoordhuis
Copy link
Member

I'm reasonably sure this fixes #128 but I can't test it right now because nodejs/node#30724 appears to have broken the Windows CI...

Fixes: #128

@nodejs-ci nodejs-ci force-pushed the canary branch 2 times, most recently from 90f86cb to d345bf4 Compare January 3, 2020 07:48
@bnoordhuis
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/32924/

So... it now compiles but fails with the following msbuild error:

13:13:49   v8_initializers.vcxproj -> ..\..\out\Release\lib\v8_initializers.lib
13:13:49 C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\bin\Microsoft.Common.CurrentVersion.targets(2850,5): error MSB4018: The "AssignTargetPath" task failed unexpectedly. [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]
13:13:49 C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\bin\Microsoft.Common.CurrentVersion.targets(2850,5): error MSB4018: System.ArgumentException: Illegal characters in path. [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]

(What illegal characters? Try and be helpful, msbuild...)

The build generates a lot of warnings along these lines:

13:13:44 c:\workspace\node-compile-windows\node\out\release\obj\global_intermediate\deps\v8\third_party\v8\builtins\array-sort-tq-csa.cc(20757): warning C4506: no definition for inline function 'v8::internal::OrderedHashSet v8::internal::OrderedHashSet::cast(v8::internal::Object)' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]
13:13:44 c:\workspace\node-compile-windows\node\out\release\obj\global_intermediate\deps\v8\third_party\v8\builtins\array-sort-tq-csa.cc(20757): warning C4506: no definition for inline function 'v8::internal::OrderedHashMap v8::internal::OrderedHashMap::cast(v8::internal::Object)' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]

That's because of this line, the implementation probably should have been in the -inl.h file.

node-v8/deps/v8/src/objects/ordered-hash-table.h

Line 132 in d345bf4

Derived NextTable() { return Derived::cast(get(NextTableIndex())); }

@bnoordhuis
Copy link
Member Author

I put two build binlogs side-to-side (one of this PR and one of a successful build of nodejs/node) and I didn't see any glaring discrepancies. If there's an illegal file path in there somewhere it's not immediately obvious to me what it is.

@nodejs-ci nodejs-ci force-pushed the canary branch 3 times, most recently from 2b674d2 to 2392638 Compare January 6, 2020 07:48
@targos
Copy link
Member

targos commented Jan 6, 2020

trying on my computer...

@targos
Copy link
Member

targos commented Jan 6, 2020

Visual Studio doesn't give more information...

image

@targos
Copy link
Member

targos commented Jan 13, 2020

@joaocgreis any idea on how we could debug further?

@targos
Copy link
Member

targos commented Jan 13, 2020

@nodejs/platform-windows

@cclauss
Copy link

cclauss commented Jan 14, 2020

Please fix conflicts.

@bnoordhuis
Copy link
Member Author

@cclauss Rebased.

nodejs-ci and others added 14 commits January 17, 2020 11:40
@nodejs-ci
doc: update README for node-v8 repository
6dc37ee
@nodejs-ci
deps: update V8 to 8.1.208
c4dccb5
@nodejs-ci
build: reset embedder string to "-node.0"
9e7b4f1
@refack @nodejs-ci
deps: V8: un-cherry-pick bd019bd
7f68787 
Original commit message:

    [testrunner] delete ancient junit compatible format support

    Testrunner has ancient support for JUnit compatible XML output.

    This CL removes this old feature.

    [email protected],[email protected],[email protected]
    CC=​[email protected]

    Bug: v8:8728
    Change-Id: I7e1beb011dbaec3aa1a27398a5c52abdd778eaf0
    Reviewed-on: https://chromium-review.googlesource.com/c/1430065
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Michael Starzinger <[email protected]>
    Commit-Queue: Tamer Tas <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59045}

Refs: v8/v8@bd019bd

PR-URL: nodejs/node#26685
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@targos @nodejs-ci
deps: V8: silence irrelevant warnings
3b615c6 
PR-URL: nodejs/node#26685
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@ryzokuken @nodejs-ci
deps: patch V8 to run on older XCode versions
9f89082 
Patch V8 (compiler/js-heap-broker.cc) to remove the use of an optional
property, which is a fairly new C++ feature, since that requires a newer
XCode version than the minimum requirement in BUILDING.md and thus
breaks CI.

PR-URL: nodejs/node#29694
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
@cjihrig @nodejs-ci
deps: update V8's postmortem script
1d3cf1e 
This commit updates V8's postmortem metadata generation script
to support V8 7.8.

The following metadata has changed:

- v8dbg_class_JSDate__value__Object
  - The postmortem metadata generation script needed to be
    updated. No action should be required by postmortem tools.

- v8dbg_class_JSRegExp__source__Object
  - The postmortem metadata generation script needed to be
    updated. No action should be required by postmortem tools.

PR-URL: nodejs/node#29694
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
@cjihrig @nodejs-ci
deps: update V8's postmortem script
0024f43 
This commit updates V8's postmortem metadata
generation script. This commit re-exposes the
v8dbg_class_UncompiledData__inferred_name__String
constant after it moved to Torque.

PR-URL: nodejs/node#30020
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@refack @nodejs-ci
deps: V8: patch register-arm64.h
4822011 
Fixes a compilation issue on some platforms

PR-URL: nodejs/node#27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@refack @nodejs-ci
deps: V8: forward declaration of Rtl*FunctionTable
a155211 
This should be semver-patch since actual invocation is version
conditional.

PR-URL: nodejs/node#27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@joaocgreis @nodejs-ci
deps: make v8.h compatible with VS2015
ef68b9f 
There is a bug in the most recent version of VS2015 that affects v8.h
and therefore prevents compilation of addons.

Refs: https://stackoverflow.com/q/38378693

PR-URL: nodejs/node#30020
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos @nodejs-ci
build: enable backtrace when V8 is built for PPC and S390x
9e319c6 
Refs: #119
@targos @ryzokuken @nodejs-ci
tools: update V8 gypfiles for V8 8.0
763fbd9 
Co-authored-by: Ujjwal Sharma <[email protected]>
@backes @nodejs-ci
src: stop overriding deprecated V8 methods
377862f 
These methods will be removed in V8 8.1, hence we need to stop
overriding them.
@nodejs-ci nodejs-ci force-pushed the canary branch 2 times, most recently from bece906 to fda488a Compare January 21, 2020 07:51
@joaocgreis
Copy link
Member

It looks like the only thing missing here is src/objects/property-descriptor-object.h.

This fixes building for me: JaneaSystems/node@e9f7149

CI: https://ci.nodejs.org/view/All/job/node-test-commit-windows-fanned/33341/

@targos
Copy link
Member

targos commented Jan 22, 2020

Nice, thanks!

@bnoordhuis would you like to upstream the changes?

@nodejs-ci nodejs-ci force-pushed the canary branch 5 times, most recently from 410ec30 to 869917f Compare February 3, 2020 07:52
@nodejs-ci nodejs-ci force-pushed the canary branch 3 times, most recently from 148710d to 413f28a Compare February 6, 2020 07:53
@gengjiawen
Copy link
Member

This need rebase.

@gengjiawen gengjiawen mentioned this pull request Feb 27, 2020
Closed
@targos
Copy link
Member

targos commented Feb 27, 2020

Landed in canary-base with #143

@targos targos closed this Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build broken on Windows (VS2017)