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

Backport --shared to V4.x codebase to allow building as a shared library / DLL #9385

Closed
wants to merge 4 commits into
base: v4.x-staging
from

Conversation

Projects
None yet
7 participants
@sxa555
Contributor

sxa555 commented Oct 31, 2016

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

build

Description of change

Enable the --shared parameter for building node as a shared library on Linux/OS X/Windows. This is a backport of the support that's already in the V6 codebase. Reference #7687 - FYI @thealphanerd

@sxa555 sxa555 changed the title from V4.x sharedlib to Enable --shared in V4.x codebase to allow building as a shared library / DLL Oct 31, 2016

@sxa555 sxa555 changed the title from Enable --shared in V4.x codebase to allow building as a shared library / DLL to Backport --shared to V4.x codebase to allow building as a shared library / DLL Oct 31, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Nov 2, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Nov 2, 2016

Member

Nit: there's a typo in the commit log message in 4bb43f7

Member

jasnell commented Nov 2, 2016

Nit: there's a typo in the commit log message in 4bb43f7

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 2, 2016

Member

@sxa555 were there other pr's for the windows support and abstraction? I don't see those in #7687

Also would it be possible for you to include the full original commit title and message from the original commit that landed, including all applicable meta data

see a722205 for an example

You can include the url to this PR as a Ref:

Member

MylesBorins commented Nov 2, 2016

@sxa555 were there other pr's for the windows support and abstraction? I don't see those in #7687

Also would it be possible for you to include the full original commit title and message from the original commit that landed, including all applicable meta data

see a722205 for an example

You can include the url to this PR as a Ref:

@sxa555

This comment has been minimized.

Show comment
Hide comment
@sxa555

sxa555 Nov 2, 2016

Contributor

Added PR Refs to description. The original PRs included multiple number of individual commits which I haven't duplicated individually for V4 - I've just got one commit for each of the three related PRs commit descriptions for the Windows and Linux ones updated to align the messages from those PRs.

Some of the things from my "abstract out" commit were done in commits within the original #6994 but for simplicity of merging what I already had for V4 I've put it all in the third of the commits. Hope that's ok

Contributor

sxa555 commented Nov 2, 2016

Added PR Refs to description. The original PRs included multiple number of individual commits which I haven't duplicated individually for V4 - I've just got one commit for each of the three related PRs commit descriptions for the Windows and Linux ones updated to align the messages from those PRs.

Some of the things from my "abstract out" commit were done in commits within the original #6994 but for simplicity of merging what I already had for V4 I've put it all in the third of the commits. Hope that's ok

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 7, 2016

Member

@sxa555 the meta data on the commits are still not the same as the original. Please use the entire original commit message.

For example 8df50ac should be changed from

 build: introduce configure --shared

Backport support for building as a shared library on Linux to V4
Ref: https://github.com/nodejs/node/pull/6994

to

 build: configure --shared

Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: https://github.com/nodejs/node/pull/6994
Ref: https://github.com/nodejs/node/pull/9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

I'm a bit concerned with the commit 7bfa0f2 which does not appear to have gone through review yet. Is this simply extra stuff that needs to be modified to make the backport work or is this completely new stuff that needs to be reviewed again?

Member

MylesBorins commented Nov 7, 2016

@sxa555 the meta data on the commits are still not the same as the original. Please use the entire original commit message.

For example 8df50ac should be changed from

 build: introduce configure --shared

Backport support for building as a shared library on Linux to V4
Ref: https://github.com/nodejs/node/pull/6994

to

 build: configure --shared

Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: https://github.com/nodejs/node/pull/6994
Ref: https://github.com/nodejs/node/pull/9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

I'm a bit concerned with the commit 7bfa0f2 which does not appear to have gone through review yet. Is this simply extra stuff that needs to be modified to make the backport work or is this completely new stuff that needs to be reviewed again?

@sxa555

This comment has been minimized.

Show comment
Hide comment
@sxa555

sxa555 Nov 7, 2016

Contributor

ref 7bfa0f2 it is a combination of the additions in 410296c to pick up the version info and the modifications that were requested in af2d04b. I'll add the commit messages tomorrow - cheers for the info

Contributor

sxa555 commented Nov 7, 2016

ref 7bfa0f2 it is a combination of the additions in 410296c to pick up the version info and the modifications that were requested in af2d04b. I'll add the commit messages tomorrow - cheers for the info

Show outdated Hide outdated vcbuild.bat
@bnoordhuis

LGTM with a style nit.

Show outdated Hide outdated src/node.h
Show outdated Hide outdated deps/v8/build/toolchain.gypi
@mhdawson

Looks to me like there are some changes in this PR which are not in the original PRs, unless I'm looking incorrectly as noted in line comments. I think a backport should just be the original changes with the only additional changes being those needed to make them apply/work.

If thats the case and I'm misunderstanding just let me know.

Show outdated Hide outdated tools/install.py
Show outdated Hide outdated configure
Show outdated Hide outdated vcbuild.bat
@sxa555

This comment has been minimized.

Show comment
Hide comment
@sxa555

sxa555 Nov 9, 2016

Contributor

Have now rebased so it merges with the 4.6.2 changes

Contributor

sxa555 commented Nov 9, 2016

Have now rebased so it merges with the 4.6.2 changes

Show outdated Hide outdated node.gyp
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Nov 9, 2016

Member

Looks consistent with v6.x except for the comment above with respect to the line

'node_use_static_libstdcpp%': 'false', in v6
Member

mhdawson commented Nov 9, 2016

Looks consistent with v6.x except for the comment above with respect to the line

'node_use_static_libstdcpp%': 'false', in v6

sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 16, 2016

build: abstract out shared library suffix
WIP: Add soname & fix make install

PR-URL: nodejs#6994
Ref: nodejs#9385
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

The build system currently creates a shared library on OS X with the
same name as on Linux i.e.  libnode.so.48.  This is inconsistent with
the conventions on OS X which uses libnode.48.dylib This commit changes
the build process and install.py (used by make binary) to build with
the correct name on OS X when the --shared configure parameter is used.

PR-URL: nodejs#7687
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 16, 2016

build: configure --shared
Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: nodejs#6994
Ref: nodejs#9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Reference:  nodejs/node#7487

sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 16, 2016

build: abstract out shared library suffix
PR-URL: nodejs#7687
Ref: nodejs#9385
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

The build system currently creates a shared library on OS X with the
same name as on Linux i.e.  libnode.so.48.  This is inconsistent with
the conventions on OS X which uses libnode.48.dylib This commit changes
the build process and install.py (used by make binary) to build with
the correct name on OS X when the --shared configure parameter is used.

PR-URL: nodejs#7687
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@sxa555 sxa555 closed this Nov 17, 2016

@sxa555 sxa555 reopened this Nov 17, 2016

@sxa555

This comment has been minimized.

Show comment
Hide comment
@sxa555

sxa555 Nov 17, 2016

Contributor

Updated with the V8 change (including version bump) as a separate commit as requested

Contributor

sxa555 commented Nov 17, 2016

Updated with the V8 change (including version bump) as a separate commit as requested

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 17, 2016

Member

hey @sxa555 sorry for making this so difficult. I ended up fixing the backport on v6.x and have landed a proper backport, with appropriate meta data, in 5bb2968

As such you can you please remove 00052e5 from this PR and rebase against the staging branch. After doing that I'll want to run the test suite one more time and then this will be ready to land!

edit: IGNORE ALL OF THIS

Member

MylesBorins commented Nov 17, 2016

hey @sxa555 sorry for making this so difficult. I ended up fixing the backport on v6.x and have landed a proper backport, with appropriate meta data, in 5bb2968

As such you can you please remove 00052e5 from this PR and rebase against the staging branch. After doing that I'll want to run the test suite one more time and then this will be ready to land!

edit: IGNORE ALL OF THIS

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 17, 2016

Member

It looks like the other patch didn't work. I bet that is the reason for the original diff... something likely landed in the gyp file of v6.x that fixed it between when it originally landed and now. This patch lands and works, so let's just stick to it.

If this CI is green I will land this PR: https://ci.nodejs.org/job/node-test-pull-request/4876/

Member

MylesBorins commented Nov 17, 2016

It looks like the other patch didn't work. I bet that is the reason for the original diff... something likely landed in the gyp file of v6.x that fixed it between when it originally landed and now. This patch lands and works, so let's just stick to it.

If this CI is green I will land this PR: https://ci.nodejs.org/job/node-test-pull-request/4876/

@sxa555

This comment has been minimized.

Show comment
Hide comment
@sxa555

sxa555 Nov 18, 2016

Contributor

test/freebsd 11 looks to be failing for other integration tests so I don't think that's related. Have pushed a fix to the linter failure by adding another space.

Contributor

sxa555 commented Nov 18, 2016

test/freebsd 11 looks to be failing for other integration tests so I don't think that's related. Have pushed a fix to the linter failure by adding another space.

@MylesBorins MylesBorins changed the base branch from v4.x to v4.x-staging Nov 18, 2016

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 18, 2016

Member

@sxa555 sorry that this process keeps dragging out. it would appear you made this PR target v4.x when it should have been targeting v4.x-staging

When targeting the correct branch there is a conflict in node.gyp

--- a/node.gyp
+++ b/node.gyp
@@@ -94,12 -99,10 +99,19 @@@
        'deps/v8/tools/tickprocessor-driver.js',
      ],
      'conditions': [
++<<<<<<< HEAD
 +      [ 'OS=="win" and '
 +        'node_use_openssl=="true" and '
 +        'node_shared_openssl=="false"', {
 +        'use_openssl_def': 1,
 +      }, {
 +        'use_openssl_def': 0,
++=======
+       [ 'node_shared=="true"', {
+         'node_target_type%': 'shared_library',
+       }, {
+         'node_target_type%': 'executable',
++>>>>>>> build: configure --shared
        }],
      ],
    },

Once the conflict is resolved, we can run the CI one last time. Thank you for your patience

Member

MylesBorins commented Nov 18, 2016

@sxa555 sorry that this process keeps dragging out. it would appear you made this PR target v4.x when it should have been targeting v4.x-staging

When targeting the correct branch there is a conflict in node.gyp

--- a/node.gyp
+++ b/node.gyp
@@@ -94,12 -99,10 +99,19 @@@
        'deps/v8/tools/tickprocessor-driver.js',
      ],
      'conditions': [
++<<<<<<< HEAD
 +      [ 'OS=="win" and '
 +        'node_use_openssl=="true" and '
 +        'node_shared_openssl=="false"', {
 +        'use_openssl_def': 1,
 +      }, {
 +        'use_openssl_def': 0,
++=======
+       [ 'node_shared=="true"', {
+         'node_target_type%': 'shared_library',
+       }, {
+         'node_target_type%': 'executable',
++>>>>>>> build: configure --shared
        }],
      ],
    },

Once the conflict is resolved, we can run the CI one last time. Thank you for your patience

stefanmb and others added some commits Jul 20, 2016

build: cherry pick V8 change for windows DLL support
PR-URL: #8084
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
build: configure --shared
Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: #6994
Ref: #9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Reference:  nodejs/node#7487
build: abstract out shared library suffix
WIP: Add soname & fix make install

PR-URL: #6994
Ref: #9385
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

The build system currently creates a shared library on OS X with the
same name as on Linux i.e.  libnode.so.48.  This is inconsistent with
the conventions on OS X which uses libnode.48.dylib This commit changes
the build process and install.py (used by make binary) to build with
the correct name on OS X when the --shared configure parameter is used.

PR-URL: #7687
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@sxa555

This comment has been minimized.

Show comment
Hide comment
@sxa555

sxa555 Nov 18, 2016

Contributor

Now resolved :-)

Contributor

sxa555 commented Nov 18, 2016

Now resolved :-)

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
Member

MylesBorins commented Nov 18, 2016

MylesBorins added a commit that referenced this pull request Nov 18, 2016

build: cherry pick V8 change for windows DLL support
Ref: #9385
PR-URL: #8084
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Nov 18, 2016

build: configure --shared
Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: #6994
Ref: #7487
Ref: #9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Nov 18, 2016

build: windows sharedlib support
Original Commit Message:
  Added "dll" option to vcbuild.bat
  Insure that Unix SO name is not used on Windows (i.e. produce a .dll file)
  Insure that Node and its V8 dependency link against the Visual C++ Runtime
  dynamically.
  Requires backported V8 patch, see PR 7802.

  Ref: #7802

  PR-URL: #7487
  Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
  Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added a commit that referenced this pull request Nov 18, 2016

build: abstract out shared library suffix
Originally part of 410296c abstracted out in backport

PR-URL: #9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins
Member

MylesBorins commented Nov 18, 2016

landed in 527db40...e97723b

@MylesBorins MylesBorins removed this from the 4.7.0 milestone Nov 22, 2016

@MylesBorins MylesBorins referenced this pull request Nov 22, 2016

Merged

v4.7.0 proposal #9736

BethGriggs added a commit to ibmruntimes/node that referenced this pull request Dec 7, 2016

build: cherry pick V8 change for windows DLL support
Ref: nodejs/node#9385
PR-URL: nodejs/node#8084
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment