Got node to build on VS2017 by refack · Pull Request #33 · indutny/gyp.js

refack · 2017-02-06T15:40:00Z

C:\bin\dev\node\node.exe D:\code\3party\gyp.js\bin\gyp --build Release -Iconfig.gypi -Icommon.gypi -Rnode node.gyp

refack · 2017-02-06T15:40:53Z

Squashes my three others (#30, #31, #32)

refack · 2017-02-06T15:43:44Z

    it('should preserve quotes', () => {
      assert.deepEqual(
-        win.adjustLibraries([ '"some path/lib1"', '-l"lib2"', 
+        win.adjustLibraries([ '"some path/lib1"', '-l"lib2"',


My IDE keeps eating this trailing white space...
Let it DIE ;)

refack · 2017-02-06T15:58:37Z

> python tools\test.py
[20:17|% 100|+ 1384|-  38]: Done

indutny

Thank you for submitting this! There are some nits that need to be fixed before landing it. Let me know if you have any questions!

Should other PRs be closed, considering that this one includes commits from the others?

indutny · 2017-02-06T22:36:59Z

  } else if (process.platform === 'win32') {
-    ar = 'lib.exe';
-    ld = 'link.exe';
+    ar = gyp.bindings.which('lib.exe');


This should be supplied by setting PATH in environment file, not by using which... Why is it necessary?

🤔😕
Conceptually you should right, but it failed for me (I have multiple link.exe in my path, and ninja used the wrong one), even though environment.x64 was generated correctly.
If I fix this somehow else, a bunch of stuff can be skipped.

That's the point of environment files. If it doesn't work - would you mind posting results of which link.exe and the environment file itself?

indutny · 2017-02-06T22:38:01Z

  main.declare('ld_host', ld);
  main.declare('ldxx_host', ldxx);
  main.declare('ar_host', ar);
+


Superfluous change.

indutny · 2017-02-06T22:41:19Z

    p = p.replace(/\$!INTERMEDIATE_DIR/g, intDir);
  }

+  p = p.replace(/(\.\.[\\\/])\$\|CONFIGURATION_NAME/g, `$1${this.configDir}`);


What exactly gets there in p and how? Why does it have ../ in it?

The (faulty) use case in icu-generic.gyp assumes it knows where the output is base on configuration and calles for

'-P', '../../<(CONFIGURATION_NAME)'

My line assumes that path manipulation (../) with CONFIGURATION_NAME actually want the configuration dir, not just name.
So we get ../../out/Release and not just ../../Release.

How does it work with regular GYP?

Voodoo
MSVS at least, resolves paths for actions from the dir of the project file.
I think in our case path resolution for actions start at base of build.

Ohh, just managed to run gyp -fninja for msvs. Same place is broken.

RE: nodejs/node#11210

indutny · 2017-02-06T22:41:38Z

-    const actionRule = action.action_name + '_' + this.index;
+    const safeActionName = action.action_name.replace(/\s/g, '_');
+    global.action_names = (global.action_names || {});
+    const nameAtConfig = global.action_names[this.config] || (global.action_names[this.config] = {});


80 column limit? Does it pass make lint?

You may also just use make format to fix it.

indutny · 2017-02-06T22:42:49Z

  list.forEach((action) => {
-    const actionRule = action.action_name + '_' + this.index;
+    const safeActionName = action.action_name.replace(/\s/g, '_');
+    global.action_names = (global.action_names || {});


It should store action_names in some JS instance, perhaps NinjaMain. Using global is a no-go here. Sorry!

NP (although gyp.js is a command line tool, not a service ;) ).

It is an npm module first of all! :)

indutny · 2017-02-06T22:44:08Z

      let stack_commit_size = linker.StackCommitSize || '';
      if (stack_commit_size) stack_commit_size = ',' + stack_commit_size;
-      ldflags.push('/STACK' + stack_reserve_size + stack_commit_size);
+      ldflags.push('/STACK:' + stack_reserve_size + stack_commit_size);


Nice catch!

Not me, the linker wan't happy.

indutny · 2017-02-06T22:45:42Z

  process.stderr.write(message + '\n');
 };
+
+exports.which = (cmd, opt) => `"${which(cmd, opt)}"`;


I'd rather not use this.

indutny · 2017-02-06T22:47:53Z

-  if (version === 'auto' && env['VS100COMNTOOLS'] || version === '2010') {
-    version = '2010';
-    tools =  path.join(env.VS120COMNTOOLS, '..', '..');
+  function tryVS7() {


Could these functions be hoisted out of resolveDevEnvironment?

indutny · 2017-02-06T22:48:27Z

+win.setEnvironment = function setEnvironment(target_arch) {
+  const lines = win.resolveDevEnvironment(target_arch);
+  lines.forEach(l => {
+    const kv = l.split('=');


Could any of env vars have several =?

indutny · 2017-02-06T22:48:50Z

+  const lines = win.resolveDevEnvironment(target_arch);
+  lines.forEach(l => {
+    const kv = l.split('=');
+    process.env[kv[0]] = kv[1];


Let's return object instead of modifying global state.

I wanted this for the which later.... So pending that.

refack · 2017-02-07T05:22:36Z

Found a way to get rid of all the which (which apparently didn't even solve my problem)...

refack · 2017-02-07T16:42:27Z

Got rid of the whole which commit.
Instead I use ninjaWrap much more

refack · 2017-02-07T16:44:38Z

-  // TODO(indutny): escape quotes in res
-  return `cmd.exe /s /c "${res}"`;
+  const wrap = gyp.platform.win.ninjaWrap;
+  return `${wrap} powershell -Command ${res.replace(/ & /g, ' ; ')}`;


powershell is way more consistent with compound commands (e.g. {cd; xx.exe; copy xx})

refack · 2017-02-07T16:53:39Z

+win.genEnvironment = function genEnvironment(outDir, target_arch) {
+  const env = win.resolveDevEnvironment(target_arch);
+  const envBlock = Object.keys(env)
+//    .filter(key => key.match(IMPORTANT_VARS))


Taking all, less complex, maybe even works better...

Let's keep it.

Keep the filter out, yes?
Just look at what vcvarscmd.bat adds

refack · 2017-02-07T16:56:34Z

> python tools\test.py
[19:55|% 100|+ 1381|-  41]: Done

I don't know what's up with that 3 extra fails, but still...

indutny

Left few more comments. Moving in the right direction! Thank you

indutny · 2017-02-07T19:20:13Z

 const path = gyp.bindings.path;
 const process = gyp.bindings.process;

+const actionNames = {};


Still it is global here 😉 It should live in NinjaMain or whatever.

That's module scope, I thought it would be even cleaner, but sure.

Well, it should be per-instance not per-application. That's my point.

indutny · 2017-02-07T19:20:46Z

-  const ninjaWrap = `ninja -t msvc ${envFile}--`;
+win.setupNinjaWrapper = function (target_arch) {
+  const envFile = win.getEnvFileName(target_arch);
+  this.ninjaWrap = `ninja -t msvc -e ${envFile} --`;


Global change again 😉

Could return a function instead.

indutny · 2017-02-07T19:22:05Z

+win.genEnvironment = function genEnvironment(outDir, target_arch) {
+  const env = win.resolveDevEnvironment(target_arch);
+  const envBlock = Object.keys(env)
+//    .filter(key => key.match(IMPORTANT_VARS))


Let's keep it.

refack · 2017-02-07T22:50:09Z

+      targetDict,
+      ninjas,
+      config: this.config,
+      actionNames,


Trickling these two into every Ninja

Heh, true. Could we use actionNames: actionNames here?

Don't you like "Object literals Shorthand property names" that's even ES2015

refack · 2017-02-09T22:28:28Z

I added another commit with some more windows black magic for finding vs2017 in restricted environments.

refack · 2017-02-13T22:52:53Z

Would you consider adding this as a dep?

indutny · 2017-02-15T18:52:15Z

That sounds good to me!

indutny · 2017-02-15T18:52:47Z

 npm-debug.log
 out/
 coverage/
+tools/*.exe


This is no longer needed, right?

indutny · 2017-02-15T18:53:55Z

  }

+  // First one is in order to outsmart silly GYP file writers
+  p = p.replace(/(\.\.[\\\/])+\$\|CONFIGURATION_NAME/g, productDir);


I'm really afraid of doing this. Could we have a test case for this?

Will write some

I wrote one, but it really concocted...

Alright, so what was the exact problem? Which path did it get here, and what was the right path to expect?

So the GYP is:

{ # build final .dat -> .obj 'action_name': 'genccode', 'inputs': [ '<(SHARED_INTERMEDIATE_DIR)/icutmp/icudt<(icu_ver_major)<(icu_endianness).dat' ], 'outputs': [ '<(SHARED_INTERMEDIATE_DIR)/icudt<(icu_ver_major)<(icu_endianness)_dat.obj' ], 'action': [ '../../<(CONFIGURATION_NAME)/genccode', '-o', '-d', '<(SHARED_INTERMEDIATE_DIR)/', '-n', 'icudata', '-e', 'icusmdt<(icu_ver_major)', '<@(_inputs)' ], }

The result (pre patch) is:

rule genccode command = cmd.exe$ /s$ /c$ "cd$ ..\..\tools\icu$ &$ ..\..\Release\genccode$ $ -o$ -d$ ..\..\out\Release\gen\$ -n$ icudata$ -e$ icusmdt58$ $ ..\..\out\Release\gen\icutmp\icudt58l.dat"

They are trying to reach the output dir, but the path voodoo fails, ..\..\Release\genccode is not there, it's at ..\..\out\Release\genccode

so after patch:

rule genccode command = cmd.exe$ /s$ /c$ "cd$ ..\..\tools\icu$ &$ $ ..\..\out\Release\genccode$ -o$ -d$ ..\..\out\Release\gen\$ -n$ icudata$ $ -e$ icusmdt58$ ..\..\out\Release\gen\icutmp\icudt58l.dat"

I made a PR to fix the root nodejs/node#11217

So this should not work on Unixes either as it is right now, right?

Not with ninja

Because other than ninja the other generated output goes in the root dir, and artifacts go in .../node/Release/... or ..../node/Debug/... and so ../../ in the path voodoo is base of root source (.../node/)

indutny · 2017-02-15T18:55:04Z

+    let targetDict = this.targetDicts[target].configurations[this.config];
    const ninja = new Ninja({
-      index: index,
+      index,


Let's leave it as it is.

indutny · 2017-02-15T18:55:20Z

+      targetDict,
+      ninjas,
+      config: this.config,
+      actionNames,


Heh, true. Could we use actionNames: actionNames here?

indutny · 2017-02-15T18:56:30Z

+  return ret.find(v => v[0] === maxVer)[1];
+}
+
+win._forTesting = {tryVS7_powershell, tryVS7_CSC, tryVS7_registry};


I guess they can be just put in exports.

indutny · 2017-02-15T18:57:29Z

  }
+  const env = lines.reduce((s, l) => {
+    const kv = l.split('=');
+    s[kv[0]] = kv[1];


Why not keep only important lines?

Who are we to judge, newer version of VS might add more variables (actually I think they did in 2017), the VsDevCmd.bat runs for almost 20 seconds to setup the ENV, it probably knows what it's doing... and anyway ninja does some more environment voodoo with the -t msvs -e xxx wrapper

I just don't want to get any unwanted extras from default global env variables.

We could do a post - pre delta calculation. I use some of these delta file my day2day dev environment.

indutny · 2017-02-15T18:58:03Z

 };

-win.genEnvironment = function genEnvironment(outDir, version, arch) {
+function tryVS7_powershell() {


Camel case? Considering that all of these functions are prefixed with tryVS7, should we put them in win/vs7.js folder?

I'm moving them into get-vs2017-path

indutny · 2017-02-15T18:59:10Z

    home_dot_gyp: homeDotGyp,
    root_targets: options.root_targets,
-    target_arch: cmdlineDefaultVariables['target_arch'] || ''
+    target_arch: cmdlineDefaultVariables['target_arch'] || 'ia32'


Let's talk about why is this necessary. 😉

So It won't be necessary in every access to conf['target_arch'] later

Should we put process.arch here then?

Sure, why not

indutny · 2017-02-16T00:31:28Z

    home_dot_gyp: homeDotGyp,
    root_targets: options.root_targets,
-    target_arch: cmdlineDefaultVariables['target_arch'] || ''
+    target_arch: cmdlineDefaultVariables['target_arch'] || 'ia32'


Should we put process.arch here then?

indutny · 2017-02-16T00:31:53Z

+if (process.platform === 'win32') {
+  const getVS2017Path = require('get-vs2017-path').getVS2017Path;
+  exports.getVS2017Path = function (hostBits, target_arch) {
+    return getVS2017Path(hostBits, target_arch, exports);


What is the last argument here for?

I name it "require less" I use your external bindings (execSync, log, and path)

indutny · 2017-02-16T00:33:05Z

-      targetDict: this.targetDicts[target].configurations[this.config],
-      ninjas: ninjas,
-      config: this.config
+      target,


Not that I mind that much, but I still have hopes for transpiling it to ES5 with a minimal AST transformation. Let's keep these changes out for now.

indutny · 2017-02-16T00:34:37Z

  }
+  const env = lines.reduce((s, l) => {
+    const kv = l.split('=');
+    s[kv[0]] = kv[1];


I just don't want to get any unwanted extras from default global env variables.

refack · 2017-02-21T14:52:16Z

we could always test build node with 'icu_small': 'false',

refack · 2017-02-21T15:05:03Z

-  // TODO(indutny): escape quotes in res
-  return `cmd.exe /s /c "${res}"`;
+  const nw = gyp.platform.win.getNinjaWrapper(this.targetArch);
+  return `${nw} cmd.exe /s /c "${res}"`;


I lost this at some point during rebasing. Just brought it back...

RE your TODO: the /s switch takes care of all your " worries.

/S behavior is to see if the first character is a quote character and if so, strip the leading character and remove the last quote character on the command line, preserving any text after the last quote character.

So res = '" "' works? This will look like nw cmd.exe /s /c " " " "...

Yep. also res = 'xxx "yyy" zzz'.
as long as res is well formed

So who will separate spaces?

What if res = 'gaga" "lala'?

I'm not sure that's well formed (in the sense pasting gaga" "lala in a windows console works), but if it is, it'll work.
Thank god it's not VBScript

cmd /s /c "xxxxxxxxxx" was meant to be like bash -- xxxxxxxxxxxxxx

Aaah, ok then :)

refack · 2017-02-21T15:07:34Z

P.S. https://ci.appveyor.com/project/refack/get-vs2017-path for it to pass, it has to pass gyp.js's test suite.

indutny · 2017-02-23T01:19:08Z

Sorry for delay.

I spent some additional time on this and now I realize that there is a problem with using external get-vs2017-path module.

The bindings file that we have in this repo is meant as an abstraction for things that are not provided by JS in general. The idea here is that gyp.js could potentially run on duktape with few C functions added to make it work on all platforms.

I'm afraid, this won't work too well for external module. Do you think there is still a chance of having it here?

refack · 2017-02-23T16:17:12Z

I'm ahead of you. Check out line 51 in binding.js, I take all dependencies from you (only need path and execSync)
It's a very only Design Pattern, called "Dependency Injection" (node.js in general used the "Registry" pattern with require())

refack · 2017-02-23T16:24:57Z

P.S. IMHO ducktapeing is a bit of overkill... making node a dependency for node building sound sane. Also gpy.js would save a ton of trouble with just node-gyp as is. But no harm in not causing too much incompatibilities.

indutny

Few very last nits. Good work, mate!

indutny · 2017-02-23T16:33:17Z

-  // TODO(indutny): escape quotes in res
-  return `cmd.exe /s /c "${res}"`;
+  const nw = gyp.platform.win.getNinjaWrapper(this.targetArch);
+  return `${nw} cmd.exe /s /c "${res}"`;


So res = '" "' works? This will look like nw cmd.exe /s /c " " " "...

indutny · 2017-02-23T16:33:27Z

+  /******************* late require *******************/
+  const getter = require('get-vs2017-path');
+  // We just need all the binding to be set on `exports`
+  getter.setBindings(exports);


indutny · 2017-02-23T16:33:48Z

  process.stderr.write(message + '\n');
 };
+
+Object.defineProperty(exports, 'win', {get() {


Let's be a bit more verbose: get: function get() {. Also, please put it on separate line and indent properly.

indutny · 2017-02-23T16:33:51Z

  process.stderr.write(message + '\n');
 };
+
+Object.defineProperty(exports, 'win', {get() {


Let's be a bit more verbose: get: function get() {. Also, please put it on separate line and indent properly.

indutny · 2017-02-23T16:33:58Z

 };
+
+Object.defineProperty(exports, 'win', {get() {
+  /******************* late require *******************/


// instead of /* */

indutny · 2017-02-23T16:34:07Z

    root_targets: options.root_targets,
-    target_arch: cmdlineDefaultVariables['target_arch'] || ''
+    target_arch: cmdlineDefaultVariables['target_arch']
+      || gyp.bindings.process.arch


|| at the end of previous line.

indutny · 2017-02-23T16:34:40Z

+  fs.writeFileSync(path.join(outDir, envFile), envBlock);
  return envFile;
 };
+


Please remove this extra whitespace.

refack · 2017-02-23T17:20:03Z

TODO: write and integration for GitHub code reviews and WebStorm 😉 initial GraphQL gist

refack · 2017-02-23T17:32:25Z

Done

refack · 2017-02-24T16:45:26Z

I renamed my package, and I'm planning on eating more of your Windows specific code. My goal is to generate window.ninja that has all the win-kruft, and that you could just #include in the generated build.ninja

indutny · 2017-03-17T16:21:39Z

@refack yikes, sorry! I forgot about this. Would you care to rebase it? This should mostly involve renaming files from lib/ to src/. Many thanks!

refack · 2017-03-17T16:22:47Z

Sure.
BTW: vote for me at nodejs/node#11852

indutny · 2017-03-17T16:35:10Z

@refack what kind of vote do you need? 😉

refack · 2017-03-17T16:57:24Z

review or say LGTW (vs. the "competing" nodejs/node#11867)

and harden ninja's `cmd` wrap mechanism

refack · 2017-03-17T17:23:23Z

Was very easy git is magic.

indutny · 2017-03-17T17:27:40Z

Landed in 259294b, thank you for awesome work!

AndrewPardoe · 2017-04-20T22:34:53Z

The officially supported vswhere tool is now available as part of the install starting today with Visual Studio 15.2 preview 2: https://blogs.msdn.microsoft.com/heaths/2017/04/21/vswhere-is-now-installed-with-visual-studio-2017/. I hope this helps your scenario.

This doesn't fix all the existing installs of VS 2017 but going forward you can now detect the VS install locations and whether C++ tools are available from a scripted environment.

refack commented Feb 6, 2017

View reviewed changes

refack force-pushed the master branch from 0d74c61 to f72ef14 Compare February 6, 2017 16:05

indutny requested changes Feb 6, 2017

View reviewed changes

refack force-pushed the master branch from f72ef14 to ecff6ea Compare February 7, 2017 15:58

refack commented Feb 7, 2017

View reviewed changes

refack force-pushed the master branch from ff92e13 to 30e875e Compare February 7, 2017 16:52

refack commented Feb 7, 2017

View reviewed changes

indutny requested changes Feb 7, 2017

View reviewed changes

refack force-pushed the master branch from 30e875e to 5c1a60a Compare February 7, 2017 22:48

refack commented Feb 7, 2017

View reviewed changes

This was referenced Feb 9, 2017

fix: handle edge case kludge in icu-generic.gyp #32

Closed

fix: gyp allows spaces in action names. ninja doesn't #31

Closed

win: try to find VS2017 tooling #30

Closed

refack force-pushed the master branch 3 times, most recently from e2e0ace to e267594 Compare February 10, 2017 23:09

indutny reviewed Feb 15, 2017

View reviewed changes

refack force-pushed the master branch 2 times, most recently from e9dee6e to bd98d01 Compare February 15, 2017 23:36

indutny reviewed Feb 16, 2017

View reviewed changes

refack force-pushed the master branch 2 times, most recently from 50cfd89 to 17e5b8a Compare February 16, 2017 01:52

refack force-pushed the master branch from 0094560 to 377968c Compare February 21, 2017 14:50

refack force-pushed the master branch from 377968c to f428be6 Compare February 21, 2017 15:03

refack commented Feb 21, 2017

View reviewed changes

indutny approved these changes Feb 23, 2017

View reviewed changes

refack force-pushed the master branch from f428be6 to 722d0a6 Compare February 23, 2017 17:31

refack force-pushed the master branch 2 times, most recently from e552dbf to f98081e Compare February 24, 2017 16:41

refack and others added 6 commits March 17, 2017 13:00

fix: gyp allows spaces in action names. ninja doesn't

cc840b3

ninja: less rule name suffixes

881b355

fix-win: /STACK argument syntax

f47b241

ninja: handle 'action' -> 'process_outputs_as_sources'

2355e19

more debuggable output

3882480

win: detect VS2017 & refactor envfile generation

cccb7ce

and harden ninja's `cmd` wrap mechanism

refack force-pushed the master branch from f98081e to cccb7ce Compare March 17, 2017 17:22

indutny closed this Mar 17, 2017

Conversation

refack commented Feb 6, 2017

Uh oh!

refack commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Feb 6, 2017

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Feb 7, 2017

Uh oh!

refack commented Feb 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Feb 7, 2017

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

refack Feb 7, 2017 •

edited

Loading