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.

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

build: fix ninja build failure #12484

Closed
wants to merge 6 commits into from
Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 18, 2017

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Apr 18, 2017
@danbev
Copy link
Contributor Author

danbev commented Apr 18, 2017

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
Copy link
Contributor

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

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'<(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)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??? 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

node.gyp Outdated
'conditions': [
['GENERATOR=="ninja"', {
'OBJ_PATH': '<(OBJ_DIR)/src/',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@refack
Copy link
Contributor

refack commented Apr 18, 2017

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
Copy link
Contributor Author

danbev commented Apr 18, 2017

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
Copy link
Contributor

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

@danbev
Copy link
Contributor Author

danbev commented Apr 19, 2017

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
Copy link
Contributor

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

@danbev
Copy link
Contributor Author

danbev commented Apr 19, 2017

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
Copy link
Contributor Author

danbev commented Apr 19, 2017

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'<(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)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@refack
Copy link
Contributor

refack commented Apr 19, 2017

@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
Copy link
Contributor

refack commented Apr 19, 2017

@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
Copy link
Contributor

refack commented Apr 19, 2017

@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
Copy link
Contributor Author

danbev commented Apr 20, 2017

@danbev
Copy link
Contributor Author

danbev commented Apr 20, 2017

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
Copy link
Member

gibfahn commented Apr 20, 2017

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
Copy link
Member

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
Copy link
Member

gibfahn commented Apr 20, 2017

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
Copy link
Member

gibfahn commented Apr 20, 2017

@refack see #12531 regarding GYP errors on macOS.

@bnoordhuis
Copy link
Member

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
Copy link
Contributor

refack commented Apr 20, 2017

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
Copy link
Contributor

refack commented Apr 20, 2017

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
Copy link
Contributor Author

danbev commented Apr 20, 2017

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
Copy link
Contributor

refack commented Apr 20, 2017

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?

refack pushed a commit that referenced this pull request May 11, 2017
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: #12484
Fixes: #12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented May 11, 2017

@refack Thanks!

@refack
Copy link
Contributor

refack commented May 11, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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 type of object separator
used for this case.

Currently the check for if ninja is being used is a normal if statement
as are the following os checks (win and aix). But the win and aix ones
should only be evaluated if the build is not generated by ninja.
This commit turns this logic into an if ninja else statement.

PR-URL: nodejs#12484
Fixes: nodejs#12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: nodejs#12484
Fixes: nodejs#12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 20, 2017

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

@danbev danbev deleted the ninja-build branch June 28, 2017 05:37
refack pushed a commit to refack/node that referenced this pull request Jul 18, 2017
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: nodejs#12484
Fixes: nodejs#12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@refack refack added the embedding Issues and PRs related to embedding Node.js in another project. label Aug 17, 2017
refack pushed a commit to refack/node that referenced this pull request Aug 23, 2017
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: nodejs#12484
Fixes: nodejs#12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: nodejs/node#12484
Fixes: nodejs/node#12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: nodejs/node#12484
Fixes: nodejs/node#12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: #12484
Fixes: #12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: #12484
Fixes: #12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
rvagg pushed a commit to nodejs/node-gyp that referenced this pull request Aug 9, 2018
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: nodejs/node#12484
Fixes: nodejs/node#12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
rvagg pushed a commit to nodejs/node-gyp that referenced this pull request Aug 9, 2018
Currently the files specified in libraries in node.gyp `cctest` target are
getting a '.lib' extension on windows when generated with ninja.
This commit adds a check to see if a file has a '.obj' extension and in
that case no '.lib' extension will be added.

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.

PR-URL: nodejs/node#12484
Fixes: nodejs/node#12448
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. embedding Issues and PRs related to embedding Node.js in another project. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ninja build failure on master
9 participants