GeezerVison

@nodejs/lts @jbergstroem

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

@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:

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

@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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

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?

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

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

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 5bb29683a5fe7600026fc8ea3747d707376dd31e

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

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/

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.

@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

Now resolved :-)

One more CI run

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

landed in 527db40...e97723b