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: don't add libraries when --enable-static #14837

Closed
wants to merge 3 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 15, 2017

Currently when building with --enabled-static the cctest target will
include libraries to be linked regardless. This commit adds a condition
to only add the libraries when dynamically linking.

Fixes: #13500

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

Currently when building with --enabled-static the cctest target will
include libraries to be linked regardless. This commit adds a condition
to only add the libraries when dynamically linking.

Fixes: nodejs#13500
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 15, 2017
@danbev
Copy link
Contributor Author

danbev commented Aug 15, 2017

Copy link
Member

@bnoordhuis bnoordhuis 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 suppose, but that 'libraries' list is such a hideous hack.

@danbev
Copy link
Contributor Author

danbev commented Aug 15, 2017

LGTM, I suppose, but that 'libraries' list is such a hideous hack.

I'll take another look at this and see how this could be done differently. I'll do that as a separate PR after sorting out the failing test when using --enable-static.

@refack
Copy link
Contributor

refack commented Aug 15, 2017

LGTM, I suppose, but that 'libraries' list is such a hideous hack.

All's fair in love and GYP. For MSVS it circumvents the horrible project dependencies, so I don't hate it.

@danbev if you're up to it, for cleanliness you could define the list as a variable:

'variables': {
  ...
  'obj_files': [		
    '<(OBJ_GEN_PATH)<(OBJ_SEPARATOR)node_javascript.<(OBJ_SUFFIX)',		
    '<(OBJ_PATH)<(OBJ_SEPARATOR)node_debug_options.<(OBJ_SUFFIX)',
    ...
  ]
  ...
}
...
     'conditions': [
        ['node_target_type!="static_library"', {
          'libraries': [ '<@(obj_files)' ]
          ...

@danbev
Copy link
Contributor Author

danbev commented Aug 15, 2017

The test failure I mentioned above was test/parallel/test-process-config.js which compares the configuration in config.gypi with the one compiled. But when using --enable-static no node executable is created and when the test run they are using the previous version (if any) and there is a difference, hence the test failure. I leaning towards this is a non issue and not worth the time coming up with a workaround.

@refack
Copy link
Contributor

refack commented Aug 15, 2017

@danbev pushed a suggested workaround for the test. Feel free to force push it out...
Quick (and probably irrelevant) sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/7999/

@danbev
Copy link
Contributor Author

danbev commented Aug 16, 2017

@danbev pushed a suggested workaround for the test. Feel free to force push it out...
Quick (and probably irrelevant) sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/7999/

Thanks @refack. The workaround I had in mind was to still create the node executable which is not created when building with --enable-static. If node was not built previously there will be an error earlier as there will be no out/Release/node. The idea I had was to perhaps have a conditional target that would build an executable even when --enable-static is used, but that executable would link to the static library libnode.a. But I'm not sure if this is worth the effort.

@refack
Copy link
Contributor

refack commented Aug 16, 2017

If node was not built previously there will be an error earlier as there will be no out/Release/node.

Ack. Obviously...

The idea I had was to perhaps have a conditional target that would build an executable even when --enable-static is used, but that executable would link to the static library libnode.a. But I'm not sure if this is worth the effort.

I like that a lot (also for dynamic_library building) but I agree that's out of scope for this PR.
See #14158 I'm trying the get the people of @electron to help with that, but if you have the "spare cycles" to take that on, that would be great. We could setup a CI job that makes sure it doesn't rot.

danbev added a commit to danbev/node that referenced this pull request Aug 17, 2017
Currently when building with --enabled-static the cctest target will
include libraries to be linked regardless. This commit adds a condition
to only add the libraries when dynamically linking.

PR-URL: nodejs#14837
Fixes: nodejs#13500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev
Copy link
Contributor Author

danbev commented Aug 17, 2017

Landed in be63c26

@danbev danbev closed this Aug 17, 2017
@danbev danbev deleted the enable-static-cctest branch August 17, 2017 05:22
@bnoordhuis
Copy link
Member

bnoordhuis commented Aug 17, 2017

I suspect this PR of breaking the Windows build with the following error:

LINK : fatal error LNK1181: cannot open input file 'c:\workspace\node-compile-windows\label\win-vs2015\Release\obj\node\gen\node_javascript.o'

Revert CI to verify: https://ci.nodejs.org/job/node-test-commit-windows-fanned/11154/

edit: yep, it's this PR.

@danbev
Copy link
Contributor Author

danbev commented Aug 17, 2017

I suspect this PR of breaking the Windows build with the following error

I'm looking into it now, thanks

@refack refack added the embedding Issues and PRs related to embedding Node.js in another project. label Aug 17, 2017
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants