deps,win: set MSVS .obj folders in gyp for V8 #13959

jaimecbernardo · 2017-06-28T14:39:37Z

When V8 was updated to 5.7 and 5.8, there were issues compiling on Windows because of what was later discovered to be a filename conflict. This PR should avoid the issue in case it reoccurs when updating V8, without the need to change the number of shards.

Building on Windows fails depending on the result from sharding the deps/v8/src/v8.gyp:v8_base target. If two source files with the same name are in the same shard, their output object file path would conflict with one another. One example of this conflict is v8_base's runtime/runtime.cc and the V8 inspector's protocol/Runtime.cpp that is generated at build time, for which the files runtime.obj and Runtime.obj would be created, but MSVS overwrites one of them with the other.

Dividing the .obj output path by the original source's extension prevents this overwrite.

Refs: #12784
Refs: #12184
Refs: #11752
Fixes: nodejs/v8#4

/cc @nodejs/v8

Checklist

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

Affected core subsystem(s)

deps,V8,build,windows

Building on Windows fails depending on the result from sharding the deps/v8/src/v8.gyp:v8_base target. If two source files with the same name are in the same shard, their output object file path would conflict with one another. One example of this conflict is v8_base's runtime/runtime.cc and the V8 inspector's protocol/Runtime.cpp that is generated at build time, for which the files runtime.obj and Runtime.obj would be created, but msvs overwrites one of them with the other. Dividing the .obj output path by the original source's extension prevents this overwrite. Fixes: nodejs/v8#4

bzoz · 2017-06-28T14:44:30Z

/cc @nodejs/v8

joaocgreis · 2017-06-28T15:53:16Z

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/8850/
CI with this change cherry-picked over the head of the 5.7 update PR (was failing on Windows x86, which was not run in CI at the time of the PR): https://ci.nodejs.org/job/node-test-commit/10796/
- There was no support for VS2017, so the win-vs2017 fails as expected.

LGTM from the Windows build side, but this should also get a review from someone with experience maintaining V8. This seems to be a more robust solution than the one I presented in #12184.

refack

LGTM from a GYP point of view

ofrobots · 2017-06-28T16:21:12Z

Can we make this change upstream first?

refack · 2017-06-28T16:22:08Z

deps/v8/src/v8.gyp

+          # .cpp files in the same shard.
+          'msvs_settings': {
+            'VCCLCompilerTool': {
+              'ObjectFile':'$(IntDir)/%(Extension)/',


One request: replace last / with \\ (GYP shenanigans 🤷‍♂️ )

Sorry, even worse $(IntDir)%(Extension)\\

So, you mean you want the line changed like this: 'ObjectFile':'$(IntDir)%(Extension)\\',?

Testing it locally and then I'll submit a commit.

Thanks for the feedback. Changes committed: 4ccd87b

refack · 2017-06-28T16:27:27Z

Can we make this change upstream first?

Worth a shot, but we need to get used to maintain the V8 .gyp files on our own...
RE: https://docs.google.com/document/d/1gvHuesiuvLzD6X6ONddxXRxhODlOJlxgfoTNZTlKLGA

ofrobots · 2017-06-28T16:46:34Z

Worth a shot, but we need to get used to maintain the V8 .gyp files on our own...

Sure. But as long as the file exists upstream, the fix should be made there first. Otherwise you'll potentially lose it the next time the V8 version is updated. At the least it will require extra care to make sure the locally floating patches are preserved each time an update to V8 is made.

jaimecbernardo · 2017-06-28T16:55:17Z

Can we make this change upstream first?

@ofrobots would renaming one of the conflicting runtime files instead be a viable solution upstream? Do you know if other projects have dependencies on the file names, which one is better to rename? Or I can simply submit this.

Refs: nodejs#13959 (comment)

jaimecbernardo · 2017-06-28T17:41:01Z

@refack updated, PTAL. Thanks for your feedback.

ofrobots · 2017-06-28T18:07:40Z

I personally think that the patch, as-proposed is better than possibility of renaming inspector's Runtime.cpp file as it is more future proof. /cc @eugeneo

refack · 2017-06-28T19:12:28Z

@jaimecbernardo you could move this directive to https://github.com/nodejs/node/blob/master/common.gypi#L112 I can't think of any harm in having this globaly... (Well testing locally now)

jaimecbernardo · 2017-06-28T19:22:58Z

@refack tried that, but it also needs changes to the projects that directly reference .obj files, like cctest:

node/node.gyp

Lines 562 to 638 in ef28d85

    
           'target_name': 'cctest', 
        
           'type': 'executable', 
        
           'dependencies': [ 
        
             '<(node_core_target_name)', 
        
             'deps/gtest/gtest.gyp:gtest', 
        
             'node_js2c#host', 
        
             'node_dtrace_header', 
        
             'node_dtrace_ustack', 
        
             'node_dtrace_provider', 
        
           ], 
        
           'variables': { 
        
             'OBJ_PATH': '<(OBJ_DIR)/node/src', 
        
             'OBJ_GEN_PATH': '<(OBJ_DIR)/node/gen', 
        
             'OBJ_TRACING_PATH': '<(OBJ_DIR)/node/src/tracing', 
        
             'OBJ_SUFFIX': 'o', 
        
             'OBJ_SEPARATOR': '/', 
        
             'conditions': [ 
        
               ['OS=="win"', { 
        
                 'OBJ_SUFFIX': 'obj', 
        
               }], 
        
               ['GENERATOR=="ninja"', { 
        
                 'OBJ_PATH': '<(OBJ_DIR)/src', 
        
                 'OBJ_GEN_PATH': '<(OBJ_DIR)/gen', 
        
                 'OBJ_TRACING_PATH': '<(OBJ_DIR)/src/tracing', 
        
                 'OBJ_SEPARATOR': '/node.', 
        
               }, { 
        
                 'conditions': [ 
        
                   ['OS=="win"', { 
        
                     'OBJ_PATH': '<(OBJ_DIR)/node', 
        
                     'OBJ_GEN_PATH': '<(OBJ_DIR)/node', 
        
                     'OBJ_TRACING_PATH': '<(OBJ_DIR)/node', 
        
                   }], 
        
                   ['OS=="aix"', { 
        
                     'OBJ_PATH': '<(OBJ_DIR)/node_base/src', 
        
                     'OBJ_GEN_PATH': '<(OBJ_DIR)/node_base/gen', 
        
                     'OBJ_TRACING_PATH': '<(OBJ_DIR)/node_base/src/tracing', 
        
                   }], 
        
                 ]} 
        
               ] 
        
             ], 
        
            }, 
        
           'includes': [ 
        
             'node.gypi' 
        
           ], 
        
           'include_dirs': [ 
        
             'src', 
        
             'tools/msvs/genfiles', 
        
             'deps/v8/include', 
        
             'deps/cares/include', 
        
             'deps/uv/include', 
        
             '<(SHARED_INTERMEDIATE_DIR)', # for node_natives.h 
        
           ], 
        
           'libraries': [ 
        
             '<(OBJ_GEN_PATH)<(OBJ_SEPARATOR)node_javascript.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)node_debug_options.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)async-wrap.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)env.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)node.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)node_buffer.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)node_i18n.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)node_url.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)util.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)string_bytes.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)string_search.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)stream_base.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)node_constants.<(OBJ_SUFFIX)', 
        
             '<(OBJ_PATH)<(OBJ_SEPARATOR)node_revert.<(OBJ_SUFFIX)', 
        
             '<(OBJ_TRACING_PATH)<(OBJ_SEPARATOR)agent.<(OBJ_SUFFIX)', 
        
             '<(OBJ_TRACING_PATH)<(OBJ_SEPARATOR)node_trace_buffer.<(OBJ_SUFFIX)', 
        
             '<(OBJ_TRACING_PATH)<(OBJ_SEPARATOR)node_trace_writer.<(OBJ_SUFFIX)', 
        
             '<(OBJ_TRACING_PATH)<(OBJ_SEPARATOR)trace_event.<(OBJ_SUFFIX)', 
        
           ],

Those references can be updated to make it build, but another impact that change would have if applied globally is causing the C++ linter to show warnings due to trying to open the .cc folders created. This PR's changes applied to the v8_base target only bring the least impact overall.

refack · 2017-06-28T19:31:37Z

This PR's changes applied to the v8_base target only bring the least impact overall.

I agree that's safest. Then we need to push this upstream. If you want I can do it I have the setup and did chromium contribution dance already...

jaimecbernardo · 2017-06-28T21:45:57Z

@refack Thanks, but I want to use this opportunity to setup and learn the steps as well.

jaimecbernardo · 2017-06-29T20:17:34Z

@refack @ofrobots Opened https://chromium-review.googlesource.com/c/556599/ to get this into V8. PTAL

Building on Windows with gyp fails depending on the result from sharding the src/v8.gyp:v8_base target. If two source files with the same name are in the same shard, their output object file path would conflict with one another. One example of this conflict is v8_base's runtime/runtime.cc and the V8 inspector's protocol/Runtime.cpp that is generated at build time, for which the files runtime.obj and Runtime.obj would be created, but MSVS overwrites one of them with the other. Dividing the .obj output path by the original source's extension prevents this overwrite. Refs: nodejs/node#13959 Bug: Change-Id: I158e6178f2511297899ee50ea159f574916f903f Reviewed-on: https://chromium-review.googlesource.com/556599 Reviewed-by: Michael Achenbach <[email protected]> Commit-Queue: Michael Achenbach <[email protected]> Cr-Commit-Position: refs/heads/master@{#46354}

jaimecbernardo · 2017-06-30T12:52:34Z

This seems to have landed in V8 upstream: https://chromium.googlesource.com/v8/v8/+/3bef2af6ef198b0cd1342680bf5d7d782dc71a46
Closing this PR. Thanks for everyone's help.

refack · 2017-06-30T21:21:49Z

@refack @ofrobots Opened https://chromium-review.googlesource.com/c/556599/ to get this into V8. PTAL

Wow it landed fast, just got to it now...

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jun 28, 2017

refack approved these changes Jun 28, 2017

View reviewed changes

refack suggested changes Jun 28, 2017

View reviewed changes

fixup: replace forward slashes

4ccd87b

Refs: nodejs#13959 (comment)

refack approved these changes Jun 28, 2017

View reviewed changes

jaimecbernardo closed this Jun 30, 2017

jaimecbernardo mentioned this pull request Jun 30, 2017

Build failures on Windows with 5.7-lkgr nodejs/v8#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps,win: set MSVS .obj folders in gyp for V8 #13959

deps,win: set MSVS .obj folders in gyp for V8 #13959

jaimecbernardo commented Jun 28, 2017

bzoz commented Jun 28, 2017

joaocgreis commented Jun 28, 2017

refack left a comment

ofrobots commented Jun 28, 2017

refack Jun 28, 2017

refack Jun 28, 2017

jaimecbernardo Jun 28, 2017 •

edited

Loading

jaimecbernardo Jun 28, 2017

jaimecbernardo Jun 28, 2017

refack Jun 28, 2017

refack commented Jun 28, 2017

ofrobots commented Jun 28, 2017

jaimecbernardo commented Jun 28, 2017

jaimecbernardo commented Jun 28, 2017

ofrobots commented Jun 28, 2017

refack commented Jun 28, 2017

jaimecbernardo commented Jun 28, 2017

refack commented Jun 28, 2017

jaimecbernardo commented Jun 28, 2017

jaimecbernardo commented Jun 29, 2017

jaimecbernardo commented Jun 30, 2017

refack commented Jun 30, 2017

deps,win: set MSVS .obj folders in gyp for V8 #13959

deps,win: set MSVS .obj folders in gyp for V8 #13959

Conversation

jaimecbernardo commented Jun 28, 2017

Checklist

Affected core subsystem(s)

bzoz commented Jun 28, 2017

joaocgreis commented Jun 28, 2017

refack left a comment

Choose a reason for hiding this comment

ofrobots commented Jun 28, 2017

refack Jun 28, 2017

Choose a reason for hiding this comment

refack Jun 28, 2017

Choose a reason for hiding this comment

jaimecbernardo Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

jaimecbernardo Jun 28, 2017

Choose a reason for hiding this comment

jaimecbernardo Jun 28, 2017

Choose a reason for hiding this comment

refack Jun 28, 2017

Choose a reason for hiding this comment

refack commented Jun 28, 2017

ofrobots commented Jun 28, 2017

jaimecbernardo commented Jun 28, 2017

jaimecbernardo commented Jun 28, 2017

ofrobots commented Jun 28, 2017

refack commented Jun 28, 2017

jaimecbernardo commented Jun 28, 2017

refack commented Jun 28, 2017

jaimecbernardo commented Jun 28, 2017

jaimecbernardo commented Jun 29, 2017

jaimecbernardo commented Jun 30, 2017

refack commented Jun 30, 2017

jaimecbernardo Jun 28, 2017 •

edited

Loading