Make --shared on AIX work (shared library support) #9675

Closed
wants to merge 2 commits into
from

Projects

None yet

8 participants

@sxa555
Contributor
sxa555 commented Nov 18, 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

Shared library support was added initially in #6994 and then for Windows in #7487. The modifications to make it work on AIX were not included. This PR pulls across a GYP fix (Includes a change I made this week - https://codereview.chromium.org/2492233002 which was landed in https://codereview.chromium.org/2511733005/ so includes changes to AUTHOR and PRESUBMIT.py which are not needed for us but included because they are part of the landed change in GYP)

My intention is to backport this to v6.x and v4.x after it's landed in master.

@mscdex mscdex added the aix label Nov 18, 2016
@bnoordhuis

The status lines of both commit logs are too long.

node.gyp
- 'type': 'executable',
+ 'conditions': [
+ ['node_shared=="true"', {
+ 'target_name': 'libnode.<(node_module_version)',
@bnoordhuis
bnoordhuis Nov 18, 2016 Member

That's a weird target name. What's more, doesn't this belong in the <(node_core_target_name) target?

@sxa555
sxa555 Nov 21, 2016 edited Contributor

On AIX, node_core_target_name is set to "node_base" (see 15bcbf8 - this happens in the normal executable case so is not specific to the shared library). An intermediate static library is created with this name which is then linked into the final executable/shared library (which depends on the node_core_target_name target).

I've re-pushed to sanitise 'target_name' :-)

sxa555 added some commits Nov 14, 2016
@sxa555 @sxa555 sxa555 deps: backport GYP fix to fix AIX shared suffix
Required to support the shared library builds on AIX - this sets the
shared library suffix within GYP to .a instead of .so on AIX
My patch: https://codereview.chromium.org/2492233002/ was landed as
as part of this one which fixed some other (not required, but
included for completeness of the backport) changes:

Ref: https://codereview.chromium.org/2511733005/
8e8aa54
@sxa555 @sxa555 sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/
63143f3
@mhdawson

LGTM provided CI is green

@mhdawson
Contributor

CI looks ok, will land

@mhdawson mhdawson self-assigned this Nov 22, 2016
@mhdawson
Contributor

Landed as 1bd8716

@mhdawson mhdawson closed this Nov 22, 2016
@mhdawson mhdawson added a commit that referenced this pull request Nov 22, 2016
@sxa555 @mhdawson sxa555 + mhdawson build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: #6994/

PR-URL: #9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
1bd8716
@sam-github sam-github added a commit to sam-github/node that referenced this pull request Nov 22, 2016
@sxa555 @sam-github sxa555 + sam-github build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
43a7edb
@gibfahn
Member
gibfahn commented Nov 23, 2016 edited

@TheAlphaNerd Should be backported to v6.x and v4.x at some point in the future. Let me know if I got the labels wrong.

Edit: Got the labels wrong anyway 😢

@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 @sxa555 sxa555 + sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
30bbe83
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 @sxa555 sxa555 + sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
d139ca6
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 @sxa555 sxa555 + sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
8ad5547
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 @sxa555 sxa555 + sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
1f38087
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 @sxa555 sxa555 + sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
45684a1
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 @sxa555 sxa555 + sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
eed1719
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 @sxa555 sxa555 + sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
dd2fa95
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
c274457
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
35d98c1
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
9ae0076
@sxa555 sxa555 referenced this pull request Nov 23, 2016
Closed

Make --shared on AIX work on v6.x #9769

1 of 2 tasks complete
@sxa555 sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 23, 2016
@sxa555 @sxa555 sxa555 + sxa555 build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: nodejs#6994/

PR-URL: nodejs#9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
aeff49d
@sxa555 sxa555 referenced this pull request Nov 24, 2016
Closed

Make --shared on AIX work on v7.x #9783

1 of 2 tasks complete
@MylesBorins
Member
MylesBorins commented Nov 24, 2016 edited

@mhdawson it would appear that 8e8aa54 didn't land

edit: not my day... seems like it didn't land because that change already existed on master.

edit 2: double sigh... it landed, but without meta data

625a271

@MylesBorins MylesBorins referenced this pull request Nov 24, 2016
@sxa555 @mhdawson sxa555 + mhdawson deps: backport GYP fix to fix AIX shared suffix
Required to support the shared library builds on AIX - this sets the
shared library suffix within GYP to .a instead of .so on AIX
My patch: https://codereview.chromium.org/2492233002/ was landed as
as part of this one which fixed some other (not required, but
included for completeness of the backport) changes:

Ref: https://codereview.chromium.org/2511733005/
625a271
@MylesBorins MylesBorins added a commit that referenced this pull request Nov 24, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: #6994/

PR-URL: #9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
16af467
@mhdawson
Contributor

? It landed as 1bd8716

@mhdawson
Contributor

Darn, I guess I must have missed squashing Arg !

@Fishrock123 Fishrock123 referenced this pull request Dec 5, 2016
Merged

v7.2.1 proposal #10127

2 of 2 tasks complete
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 8, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins deps: backport GYP fix to fix AIX shared suffix
Required to support the shared library builds on AIX - this sets the
shared library suffix within GYP to .a instead of .so on AIX
My patch: https://codereview.chromium.org/2492233002/ was landed as
as part of this one which fixed some other (not required, but
included for completeness of the backport) changes:

Ref: https://codereview.chromium.org/2511733005/
PR-URL: #9675

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
1022212
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 8, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: #6994/

PR-URL: #9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
168241a
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 8, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins deps: backport GYP fix to fix AIX shared suffix
Required to support the shared library builds on AIX - this sets the
shared library suffix within GYP to .a instead of .so on AIX
My patch: https://codereview.chromium.org/2492233002/ was landed as
as part of this one which fixed some other (not required, but
included for completeness of the backport) changes:

Ref: https://codereview.chromium.org/2511733005/
PR-URL: #9675

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
ad4e051
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 8, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: #6994/

PR-URL: #9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
666a582
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 9, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins deps: backport GYP fix to fix AIX shared suffix
Required to support the shared library builds on AIX - this sets the
shared library suffix within GYP to .a instead of .so on AIX
My patch: https://codereview.chromium.org/2492233002/ was landed as
as part of this one which fixed some other (not required, but
included for completeness of the backport) changes:

Ref: https://codereview.chromium.org/2511733005/
PR-URL: #9675

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
52f1493
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 9, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: #6994/

PR-URL: #9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
f2b0098
@MylesBorins
Member

great news @sxa555

This has gone out in the latest v7 release and it landed cleanly on both v6 and v4 so no need for a manual backport!

@MylesBorins MylesBorins added a commit that referenced this pull request Dec 21, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins deps: backport GYP fix to fix AIX shared suffix
Required to support the shared library builds on AIX - this sets the
shared library suffix within GYP to .a instead of .so on AIX
My patch: https://codereview.chromium.org/2492233002/ was landed as
as part of this one which fixed some other (not required, but
included for completeness of the backport) changes:

Ref: https://codereview.chromium.org/2511733005/
PR-URL: #9675

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
19d5dad
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 21, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: #6994/

PR-URL: #9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
1256599
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 21, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins deps: backport GYP fix to fix AIX shared suffix
Required to support the shared library builds on AIX - this sets the
shared library suffix within GYP to .a instead of .so on AIX
My patch: https://codereview.chromium.org/2492233002/ was landed as
as part of this one which fixed some other (not required, but
included for completeness of the backport) changes:

Ref: https://codereview.chromium.org/2511733005/
PR-URL: #9675

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
7500f89
@MylesBorins MylesBorins added a commit that referenced this pull request Dec 21, 2016
@sxa555 @MylesBorins sxa555 + MylesBorins build: add shared library support to AIX build
Updates to build the shared library version of node on AIX. Adds the
same functionality to AIX that was added on Linux under this:

Ref: #6994/

PR-URL: #9675
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
2cd4a7f
This was referenced Dec 21, 2016
@MylesBorins MylesBorins added a commit to MylesBorins/node that referenced this pull request Jan 3, 2017
@MylesBorins MylesBorins 2016-01-03, Version 6.9.3 'Boron' (LTS) Release
This LTS release comes with 312 commits. This includes 229 that are
test related, 62 that are docs related, 17 which are build / tools
related, and 4 commits which are updates to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) nodejs#9675
* deps:
    - *npm*: upgrade npm to 3.10.10 (Rebecca Turner)
             nodejs#9847
    - *V8*: Destructuring of arrow function arguments via computed
            property no longer throws (Michaël Zasso)
            nodejs#10386)
* inspector:
  - /json/version returns object, not an object wrapped in an array
    (Ben Noordhuis) nodejs#9762
* module:
  - using --debug-brk and --eval together now works as expected
    (Kelvin Jin) nodejs#8876
* process:
  - improve performance of nextTick up to 20% (Evan Lucas)
    nodejs#8932
* repl:
    - the division operator will no longer be accidentally parsed as
      regex (Teddy Katz) nodejs#10103
    - improved support for generator functions (Teddy Katz)
      nodejs#9852
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) nodejs#9685

PR-URL: nodejs#10394
93a9109
@MylesBorins MylesBorins added a commit that referenced this pull request Jan 3, 2017
@MylesBorins MylesBorins 2016-01-03, Version 4.7.1 'Argon' (LTS)
This LTS release comes with 180 commits. This includes 117 which are
test related, 34 which are doc related, 15 which are build / tool
related, and 1 commit which is an update to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* repl:
  - Passing options to the repl will no longer overwrite defaults
    (cjihrig) #7826
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10395
b460d9f
@MylesBorins MylesBorins added a commit that referenced this pull request Jan 3, 2017
@MylesBorins MylesBorins 2017-01-03, Version 4.7.1 'Argon' (LTS)
This LTS release comes with 180 commits. This includes 117 which are
test related, 34 which are doc related, 15 which are build / tool
related, and 1 commit which is an update to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* repl:
  - Passing options to the repl will no longer overwrite defaults
    (cjihrig) #7826
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10395
1035318
@MylesBorins MylesBorins added a commit that referenced this pull request Jan 3, 2017
@MylesBorins MylesBorins 2017-01-03, Version 6.9.3 'Boron' (LTS) Release
This LTS release comes with 312 commits. This includes 229 that are
test related, 62 that are docs related, 17 which are build / tools
related, and 4 commits which are updates to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* deps:
    - *npm*: upgrade npm to 3.10.10 (Rebecca Turner)
             #9847
    - *V8*: Destructuring of arrow function arguments via computed
            property no longer throws (Michaël Zasso)
            #10386)
* inspector:
  - /json/version returns object, not an object wrapped in an array
    (Ben Noordhuis) #9762
* module:
  - using --debug-brk and --eval together now works as expected
    (Kelvin Jin) #8876
* process:
  - improve performance of nextTick up to 20% (Evan Lucas)
    #8932
* repl:
    - the division operator will no longer be accidentally parsed as
      regex (Teddy Katz) #10103
    - improved support for generator functions (Teddy Katz)
      #9852
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10394
42149d7
@MylesBorins MylesBorins added a commit that referenced this pull request Jan 3, 2017
@MylesBorins MylesBorins 2017-01-03, Version 6.9.3 'Boron' (LTS) Release
This LTS release comes with 312 commits. This includes 229 that are
test related, 62 that are docs related, 17 which are build / tools
related, and 4 commits which are updates to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* deps:
    - *npm*: upgrade npm to 3.10.10 (Rebecca Turner)
             #9847
    - *V8*: Destructuring of arrow function arguments via computed
            property no longer throws (Michaël Zasso)
            #10386)
* inspector:
  - /json/version returns object, not an object wrapped in an array
    (Ben Noordhuis) #9762
* module:
  - using --debug-brk and --eval together now works as expected
    (Kelvin Jin) #8876
* process:
  - improve performance of nextTick up to 20% (Evan Lucas)
    #8932
* repl:
    - the division operator will no longer be accidentally parsed as
      regex (Teddy Katz) #10103
    - improved support for generator functions (Teddy Katz)
      #9852
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10394
2bf1c24
@MylesBorins MylesBorins added a commit that referenced this pull request Jan 4, 2017
@MylesBorins MylesBorins 2017-01-03, Version 4.7.1 'Argon' (LTS)
This LTS release comes with 180 commits. This includes 117 which are
test related, 34 which are doc related, 15 which are build / tool
related, and 1 commit which is an update to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* repl:
  - Passing options to the repl will no longer overwrite defaults
    (cjihrig) #7826
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10395
b26a469
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment