Initial gyp.js support #962

pmed · 2016-06-19T00:39:43Z

Added a command-line switch --gypjs to use https://github.com/indutny/gyp.js instead of gyp.

Removed copyNodeLib() on Windows in favor to use target_arch variable from generated configuration file build/config.gypi

Fixes: #960

rvagg · 2016-06-20T10:38:06Z

Can you explain why the need for all the Windows changes in here? Without a solid test suite and the lack of Windows expertise around here this PR is likely going to be very difficult to land given how heavy-handed it is with those changes.

rvagg · 2016-06-20T10:39:43Z

Ah, I see #951, I'm guessing that what it's all about?

you should open one PR per concern to increase the likelihood of getting changes in, if gyp.js doesn't have any reason to depend on the node.lib location changes then they don't belong together in a PR.

pmed · 2016-06-20T12:18:10Z

On Windows node.lib library is listed in a libraries section of addon.gypi in a strange manner. The library path contains a MSBuild-specific macro $(ConfigurationName). That path is placed into a generated .ninja build file and the $(ConfigurationName) macro is obviously staying unexpanded during the building with ninja.

I wonder, is anybody use node-gyp on Windows with other than Visual C++ toolset?

As I understand copyNodeLib() function is only copies an existing <(node_root_dir)/<(target_arch)/node.lib to <(node_root_dir)/$(ConfigurationName)/node.lib which is unnecessary from my point of view.

I also added a findMSVS() function to locate Visual C++ directory and to its paths with vcvarsall.bat invocation on a build stage, so node-gyp build would work on Windows without additional actions. Probably, vcvarsall.bat should be called explicitly by users, because they may have another C++ toolsets.

Other changes are just README update and branches in build and configure commands like

if (!gyp.opts.gypjs) {
//   old code for gyp remained
} else {
//  new code for gyp.js added
}

…ndows Using `target_arch` in addon.gypi to link against Node.js library. This variable was written into build/config.gypi on the `configure` stage. Do not copy node.lib into node_root_dir/Release or node_root_dir/Debug on Windows, link it from node_root_dir/target_arch directory. Removed unused copyNodeLib() function Removed unused `copy_dev_lib` variable introduced in commit @84d24189735e19350a93aaf9f6a327bb4c52349e

pmed · 2016-06-20T13:21:49Z

@rvagg Extracted the fix with target_arch usage into a separate PR #964 so this PR should be merged after that

indutny · 2016-06-20T17:29:16Z

README.md

+$ node-gyp configure --gypjs
+```
+
+It is also possible to set `npm_config_gypjs` environment variable to turn on


I would suggest all-capitals env var name: NPM_CONFIG_GYPJS, or do we use lower-case now?

Environment variables named with prefix npm_config_ are mapped to gyp.opts object. Thus, npm_config_gypjs variable will set gyp.opts.gypjs that used later. As I know an environment variable names are case-sensitive.

Ah, ok. My bad then!

Though, they are case sensitive.

no need for this after #1185

Set path to node lib in `$(Configuration)` dir when `--nodedir` option is supplied, otherwise use value of `target_arch` variable.

indutny · 2016-06-26T08:05:02Z

Is there anything left before we can move forward on this? cc @rvagg

indutny · 2016-06-26T08:05:21Z

addon.gypi

@@ -92,7 +92,7 @@
          '-luuid.lib',
          '-lodbc32.lib',
          '-lDelayImp.lib',
-          '-l"<(node_root_dir)/$(ConfigurationName)/<(node_lib_file)"'
+          '-l"<(node_lib_file)"'


$(ConfigurationName) is a MSBuild macro unknown for Ninja. So I changed it to target_arch variable. But this won't work with --nodedir option on Windows. As a result node_lib_file contains full path to the node.lib now.

See comments in PR #964. Actually that request should be merged before this one.

For the record #964 landed

indutny · 2016-07-01T19:28:59Z

Update from me: ninja is no longer required to be present on the system, as gyp.js bundles a minimal implementation of it.

indutny · 2016-07-03T07:19:21Z

Ahem, ping?

rvagg · 2016-07-07T11:51:03Z

Let's do some @next releases for a v4 eh? I'll make a next branch and do it off there and we can keep them isolated from @latest so they are just for testing.

@pmed can you rebase this off master and squash it down to just the essential commits (or just one commit if that makes sense), it doesn't apply neatly to the current master.

pmed · 2016-07-07T14:05:57Z

@rvagg this PR is based on #964, so that pull request will be included into the squashed commit, is it ok?

rvagg · 2016-07-08T06:46:34Z

right, just leave that one out of the squash and we'll deal with it separately

See issue nodejs#960 Added initial `gyp.js` support with --gypjs command line option. Environment variable `npm_config_gypjs` also turns this option on. Update configure and build usage strings depending on `npm_config_gypjs` environment variable. Set `npm_config_gypjs` env variable if `--gypjs` command-line option was set to affect usage text for `configure` and `build` commands. Update usage strings if `--gypjs` command-line option was supplied Trying to load gyp.js module only if --gypjs command-line option was supplied.

indutny · 2016-07-19T01:27:56Z

ping @rvagg

indutny · 2016-07-19T01:28:54Z

README.md

+implementation in JavaScript. It generates [`Ninja`](https://ninja-build.org/)
+build files and requires no Python installation.
+
+In this case you will need to install [`Ninja`](https://ninja-build.org/) build


@pmed this is no longer the case. I would say "For fast and incremental builds ninja build too should be installed, otherwise a default JavaScript shim will be used to build your addon".

Though, node-gyp will probably need to be modified to use it.

Or alternatively npm install -g ninja.js may be recommended, if binary is not available.

This shouldn't be a blocker for this PR, though.

There should be a fallback to ninja.js in doWhich() checks, when ninja was not found.

What about using ninja.js directly instead?

El 2/8/2016 11:22, "Pavel Medvedev" notifications@github.com escribió:

In README.md
#962 (comment):

@@ -106,6 +106,37 @@ you can require the .node file with Node and run your tests!
Note: To create a Debug build of the bindings file, pass the --debug (or
-d) switch when running either the configure, build or rebuild command.

+#### Usage without Python
+
+There is a gyp.js project, a GYP
+implementation in JavaScript. It generates Ninja
+build files and requires no Python installation.
+
+In this case you will need to install Ninja build

There should be a fallback to ninja.js in doWhich() checks, when ninja
was not found.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node-gyp/pull/962/files/1e8a221ef69573949fc288102f1ba4bb7d048a7e#r73122036,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAgfvjh510_d3fOgLcPJXTY51EP1ACYgks5qbww6gaJpZM4I5EHk
.

Current status of ninja.js is "work-in-progress", and I've never tried it. Maybe later, in next PR after @indutny will release ninja.js

Let's do it in next PR indeed. ninja.js is kind of working, but we don't want to delay this thing.

BTW I don't know how to spawn ninja.js as a command-line utility in cross-platfrom way.

Don't worry too much about it. I'll send a PR once this will land.

indutny · 2016-08-30T21:57:30Z

@rvagg ping

lin7sh · 2016-10-10T19:29:32Z

any update?

indutny · 2016-11-05T03:18:47Z

Hello everyone!

I'd like to ping @nodejs/addon-api and @nodejs/node-gyp . Let's get this one resolved.

pmed · 2016-11-10T13:29:04Z

@indutny, as I understand, PR #964 should be accepted at first.

jbergstroem · 2016-11-10T14:12:22Z

I'd love to see this go in!

lin7sh · 2016-11-20T21:10:44Z

any update? This is a great idea, why progress so slow

jbergstroem · 2017-01-17T20:04:03Z

@bnoordhuis anything I/someone can do to see this land? Asking you since you're generally the goto node-gyp bro :)

bnoordhuis · 2017-01-18T07:49:28Z

In all honesty, I like the concept of gyp.js but it's not going to be a drop-in replacement anytime soon and I don't really see the point of merging it into node-gyp.

indutny · 2017-01-18T14:10:40Z

@bnoordhuis the point of merging it under the flag is to bring it to the point where it is going to be a drop-in replacement. Right now, as far we are aware gyp.js works pretty good on all of the addons that we tested. Having this flag will allow us to fix remaining bugs (if there are any), and move forward.

That being said, I don't see any downside of having potentially good functionality under a flag.

jbergstroem · 2017-01-18T14:21:29Z

Perhaps we can document it as "not supported"/"don't depend on it", meaning we can drop it at any release?

indutny · 2017-01-19T17:29:45Z

I agree, this totally makes sense.

@bnoordhuis care to take another look at this? cc @rvagg too

bnoordhuis · 2017-01-19T17:42:50Z

Can you sell me on it? The reason it's not a drop-in replacement is that gyp allows arbitrary python expressions (and people use that) while gyp.js does not and probably never will. Also, ninja instead of make.

indutny · 2017-01-19T17:51:16Z

@bnoordhuis there is naive ninja.js port available for gyp.js, so there is no need to have actual ninja binary on the system.

What particular python expressions are you referring to? It does support logical expressions, comparisons, and quite a lot other things that are supported too: https://github.com/indutny/gyp.js/blob/master/test/py-compile-condition-test.js . This could be extended very easily to make it support most of the common use cases.

The selling points:

No python will be generally needed to build addons (and this applies to Windows too)
It is just much faster than GYP
GYP is going away, and will not be maintained eventually

jbergstroem · 2017-01-19T18:00:19Z

In terms of logic; I feel we should not aim for "full" compatibility (whatever that means). I don't think we need to treat gyp.js as a drop-in replacement, rather a long term replacement that might require minor changes in complicated gyp-files.

indutny · 2017-01-19T18:01:47Z

Even with this target in mind, we still can make some of the edge-cases work with gyp.js . It should be pretty easy

bnoordhuis · 2017-01-19T18:21:47Z

What particular python expressions are you referring to?

10 <= int(var.strip().split(".")[0]) <= 12, var.strip().lower()[:4] in ("arm","mips"), var.replace("-", "_"), stuff like that. Diverse enough that you probably end up implementing a sizable subset of the language.

there is naive ninja.js port available for gyp.js, so there is no need to have actual ninja binary on the system.

Okay, that speaks in gyp.js's favor. :-)

indutny · 2017-01-19T18:24:24Z

@bnoordhuis Mmm.. supporting that expression doesn't seem to be really painful, we kind of support half of it already.

indutny · 2017-01-19T18:24:53Z

@bnoordhuis though, I'm sure that it may be simplified to something that is already supported, or just expanded in terms of supported expressions.

bnoordhuis · 2017-01-19T18:31:13Z

In any case, this PR will need to be rebased.

pmed · 2017-01-21T20:12:14Z

Yet another selling point for gyp.js is the node-gyp supports only Python 2.x version. So people with already Python 3.x installed would have to install Python 2.x and here they may face with difficulties of co-existing two Python versions, since they are not fully compatible.

indutny · 2017-01-21T20:29:33Z

@pmed Would you care to rebase this? #1092 was opened just in case if you wouldn't want to follow up.

pmed · 2017-01-21T23:36:15Z

@indutny You've already rebased this PR, and #1092 has some comments, so let's run it there.

refack · 2017-06-08T14:36:34Z

@pmed are you interested with moving this forward? I'd be happy to help.

refack · 2017-06-08T14:38:11Z

Superseded by #1092

This was referenced Jun 20, 2016

Copying node.lib timestamp to prevent redundant link with msbuild #951

Closed

Consider gyp.js? #960

Closed

pmed force-pushed the feature-gypjs branch from 2428dc2 to 5cd4967 Compare June 20, 2016 13:17

pmed mentioned this pull request Jun 20, 2016

build: use target_arch config variable to link against node.lib #964

Closed

indutny reviewed Jun 20, 2016
View reviewed changes

pmed force-pushed the feature-gypjs branch from 5cd4967 to 6b4a63c Compare June 20, 2016 21:09

configure: use full path in node_lib_file GYP variable

5eb0972

Set path to node lib in `$(Configuration)` dir when `--nodedir` option is supplied, otherwise use value of `target_arch` variable.

pmed force-pushed the feature-gypjs branch from 6b4a63c to c83153a Compare June 23, 2016 13:07

indutny reviewed Jun 26, 2016
View reviewed changes

lovell mentioned this pull request Jul 5, 2016

Gyp dependency on Windows despite shipping precompiled binary lovell/sharp#458

Closed

pmed force-pushed the feature-gypjs branch from 876b70a to eb2c709 Compare July 9, 2016 08:12

indutny reviewed Jul 19, 2016
View reviewed changes

indutny mentioned this pull request Jan 20, 2017

lib: add gyp.js support #1092

Open

refack mentioned this pull request Apr 9, 2017

Replace gyp, node-gyp for addons by adding compiler abstraction nodejs/node#7440

Closed

4 tasks

refack suggested changes Jun 8, 2017

View reviewed changes

refack closed this Jun 8, 2017

Initial gyp.js support #962

Initial gyp.js support #962

Conversation

pmed commented Jun 19, 2016 • edited by refack

rvagg commented Jun 20, 2016

rvagg commented Jun 20, 2016

pmed commented Jun 20, 2016 • edited

pmed commented Jun 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Jun 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Jul 1, 2016

indutny commented Jul 3, 2016

rvagg commented Jul 7, 2016

pmed commented Jul 7, 2016

rvagg commented Jul 8, 2016

indutny commented Jul 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny Aug 4, 2016 • edited

Choose a reason for hiding this comment

indutny commented Aug 30, 2016

lin7sh commented Oct 10, 2016

indutny commented Nov 5, 2016

pmed commented Nov 10, 2016

jbergstroem commented Nov 10, 2016

lin7sh commented Nov 20, 2016

jbergstroem commented Jan 17, 2017

bnoordhuis commented Jan 18, 2017

indutny commented Jan 18, 2017

jbergstroem commented Jan 18, 2017

indutny commented Jan 19, 2017

bnoordhuis commented Jan 19, 2017

indutny commented Jan 19, 2017

jbergstroem commented Jan 19, 2017

indutny commented Jan 19, 2017

bnoordhuis commented Jan 19, 2017 • edited

indutny commented Jan 19, 2017

indutny commented Jan 19, 2017

bnoordhuis commented Jan 19, 2017

pmed commented Jan 21, 2017

indutny commented Jan 21, 2017 • edited

pmed commented Jan 21, 2017

refack commented Jun 8, 2017

refack commented Jun 8, 2017

pmed commented Jun 19, 2016 •

edited by refack

pmed commented Jun 20, 2016 •

edited

pmed commented Jun 20, 2016 •

edited

indutny Aug 4, 2016 •

edited

bnoordhuis commented Jan 19, 2017 •

edited

indutny commented Jan 21, 2017 •

edited