build: fix ninja build failure

[danbev]

When working on commit 6a09a69
("build: enable cctest to use generated objects") I did not take into
account building with ninja:

$ ./configure $ tools/gyp_node.py -f ninja $ ninja -C out/Release $ ln -fs out/Release/node node

When ninja generated the ninja build files, src files that are
relative to the src directory will be named with a dot instead of a
path separator, for example:

out/Release/obj/src/node/node.o

would instead become:

out/Release/obj/src/node.node.o

This commit adds an additional variable for the typ of object separator
used for this case.

Also, the LIBS specified in the 'libraries' section are not
being included in the --start-group --end-group section which
means that these libraries will not be searched causing issue
with linkers where the order matters.

Fixes: #12448

Checklist

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

Affected core subsystem(s)

build

[danbev]

I'm not really sure if this is a good solution but want to see if this passes all the CI runs before looking into something better (perhaps making a change to ninja.py).

CI: https://ci.nodejs.org/job/node-test-pull-request/7469/

[sam-github]

Nice commit message, but " typ of object separator" has a typo ("type").

[benjamingr]

LGTM, I don't think the fix is that ugly fwiw and it fixes the problem locally for me and in the CI - but I'm not a very skilled ninja user by any means.

[refack]

Is this necessary? GYP usually knows how to handle slashes?

[gibfahn]

??? The object separator is .node on ninja or / on gyp, not / or \. This is the bit that actually fixes building with ninja, why wouldn't it be necessary?

[refack]

Sorry I missed the 'OBJ_SEPARATOR': 'node.',

[refack]

Still it probably should be 'OBJ_SEPARATOR': '/node.', with no trailing slashes in other variables.

[refack]

don't end paths with a / it breaks on Windows (when it's the end of the final argument)

[refack]

I have a feeling the slash manipulation change in node.gyp is not necessary (or might even degrade ninja output on Windows). Is there a way for you to gist a before and after outputs? for us to compare?

[danbev]

I have a feeling the slash manipulation change in node.gyp is not necessary (or might even degrade ninja output on Windows).

I'll run the ninja build on windows to verify that it works there too.

[Fishrock123]

FWIW building with ninja on master works with or without this for me. macOS 10.12.4, ninja 1.6.0.

[danbev]

FWIW building with ninja on master works with or without this for me. macOS 10.12.4, ninja 1.6.0.

Is it possible that you already have built the project, which would mean that you have existing object files in out/Release/obj.target/node/src and the ninja build would in that case succeed?
I think doing a clean will show this error.

[Fishrock123]

Ah, I thought I did make clean yesterday but apparently not. Yes, I also have the issue and this fixes it.

[danbev]

I've managed to run this on windows now and I've identified two issues. One is a simple fix where the win settings were overriding the ones set by the ninja condition.

The second issue I found was that the libraries in the cctest target in node.gyp, which in our case are object files, are getting a .lib extension in out/Release/obj/cctest.ninja. This causes a failure as there are no such files.
I've added a condition to tools/gyp/pylib/gyp/msvs_emulation.py to check for this situation. Not sure if this is a good solution or not but perhaps others can chime in now that they know the issue.

These are the commands I used to build:

> python configure --debug --ninja --dest-cpu=x86 --without-intl > tools\gyp_node.py -f ninja > ninja -C out\Release > out\Release\cctest.exe

[danbev]

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/7526/

[refack]

Try to remove trailing slashes, and use prefixed slashes instead.

[refack]

Still it probably should be 'OBJ_SEPARATOR': '/node.', with no trailing slashes in other variables.

[refack]

@danbev sorry you got into GYP hell. If When you get frustrated, my best advice, do like @bnoordhuis in #12231, move the crazy logic into a python file. You can then call it with <! or <!@https://gyp.gsrc.io/docs/InputFormatReference.md#Command-Expansions

[refack]

@danbev another tip: remember that GYP is just the Makefile generator. Running configure is cheap. Then rename output dir (out or Release) change .gyp files, rerun and text compare.

[refack]

@gibfahn@danbev@Fishrock123 I'm knee deep in the GYP code (they want the ninja generator to be VS2017 compatible), any thing I can fix while I'm there?

[danbev]

CI: https://ci.nodejs.org/job/node-test-pull-request/7544/

[danbev]

Try to remove trailing slashes, and use prefixed slashes instead.

@refack I've used a prefixed slash now, let me know what you think.

[gibfahn]

any thing I can fix while I'm there?

@refack the thing that irritates me with ninja is that if you do a ninja build and then a make test, make doesn't understand that the ninja build happened and rebuilds everything. I'm not sure how easy that would be to fix though, I suspect that might be beyond what make can do.

Also there could be a/some ninja based make target(s), but I don't know if collaborators would be okay with that.

The other thing is nodejs/node-gyp#1057, but that's mac specific, and I think upstream GYP aren't doing the xcodebuild stuff any more, so IDK if there's a way we can update gyp and avoid the warnings. In any case, warnings every time you configure are a pain.

[bnoordhuis]

Also there could be a/some ninja based make target(s), but I don't know if collaborators would be okay with that.

The Makefile used to have them but I removed it in de224d6. I don't have anything against ninja but approximately no one was using it at the time and it was slowing down the make build.

[gibfahn]

approximately no one was using it at the time and it was slowing down the make build.

How much does it slow down the build? It's interesting that people don't use it, I find it faster and cleaner to use, the only issue is that it doesn't play well with other make targets.

[gibfahn]

@refack see #12531 regarding GYP errors on macOS.

[bnoordhuis]

It's interesting that people don't use it, I find it faster and cleaner to use

I speculate that most people invoke ninja directly rather than through make (at least that is what I did - you use ninja because it's faster whereas the Makefile is a lumbering behemoth.)

[refack]

In #12425 I'm suggesting replacing make with ninja in order to eliminate the win/posix build disparity. Also it's faster (even than make).

[refack]

Code looks better, but ninja on windows fails:

D:\code\node\out.ninja\out\Debug$ ninja -j 3 -v -f build.ninja [1/3] C:\bin\dev\python27\python.exe gyp-win-tool link-with-manifests environment.x64 True node.exe "C:\bin\dev\python27\python.exe gyp-win-tool link-wrapper environment.x64 False link.exe /nologo /OUT:node.exe @node.exe.rsp"1 mt.exe rc.exe "obj\node.node.exe.intermediate.manifest" obj\node.node.exe.generated.manifest ..\..\..\src\res\node.exe.extra.manifest [2/3] C:\bin\dev\python27\python.exe gyp-win-tool stamp obj\cctest.actions_depends.stamp [3/3] C:\bin\dev\python27\python.exe gyp-win-tool link-with-manifests environment.x64 True cctest.exe "C:\bin\dev\python27\python.exe gyp-win-tool link-wrapper environment.x64 False link.exe /nologo /OUT:cctest.exe @cctest.exe.rsp"1 mt.exe rc.exe "obj\cctest.cctest.exe.intermediate.manifest" obj\cctest.cctest.exe.generated.manifest ..\..\..\src\res\node.exe.extra.manifest FAILED: cctest.exe cctest.exe.pdb C:\bin\dev\python27\python.exe gyp-win-tool link-with-manifests environment.x64 True cctest.exe "C:\bin\dev\python27\python.exe gyp-win-tool link-wrapper environment.x64 False link.exe /nologo /OUT:cctest.exe @cctest.exe.rsp"1 mt.exe rc.exe "obj\cctest.cctest.exe.intermediate.manifest" obj\cctest.cctest.exe.generated.manifest ..\..\..\src\res\node.exe.extra.manifest LINK : fatal error LNK1104: cannot open file 'obj/gen/node.node_javascript.obj.lib'

But since I have my knees deep in GYP I'm following up aswell.

[danbev]

Code looks better, but ninja on windows fails:

LINK : fatal error LNK1104: cannot open file 'obj/gen/node.node_javascript.obj.lib'

This does not look like you have the the changes in 5ddca82. Could you double check?

[refack]

This does not look like you have the the changes in 5ddca82. Could you double check?

Clashing GYP patches. I have patched that line to fix a different bug.
I feel like if you have to patch GYP in order to enable a feature (#11956), you might be doing something wrong 😉

Really no disrespect intended, GYP is super fragile, and it's baby .gyp files are super convoluted.
What do you say we roll back #11956, and work on it from a wider perspective together?

[danbev]

@refack Thanks!

[refack]

Post land CI: https://ci.nodejs.org/job/node-test-commit/9810/

[gibfahn]

Fix for #11956, so should be backported with that.