-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: Updates for AIX npm support - part 1 #3114
Conversation
#!/bin/sh | ||
echo "Searching $1 to write out expfile to $2" | ||
echo "#!." > $2 | ||
find $1 -name "*.a" |xargs nm -Xany -BCpg | awk '{ if ((($2 == "T") || ($2 == "D") || ($2 == "B")) && (substr($3,1,1) != ".")) {print $3} }' | sort -u >> $2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we clean this up a little, some missing spaces and it's quite long, maybe something like:
find $1 -name "*.a" \
| xargs nm -Xany -BCpg \
| awk '{
if ((($2 == "T") || ($2 == "D") || ($2 == "B")) && (substr($3,1,1) != ".")) { print $3 }
}' \
| sort -u >> $2
Also perhaps a comment at the top of the file that this if for creating an AIX .exp file, just to give some contents to the next poor dev who goes fishing around in tools/ and stumbles on our crazy assortment of hacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, once we settle any other issues I'll fix this up/
Would it make sense to split the current 'node' target in two: one that builds libnode.a, one that builds the binary? AIX-specific post-processing could then be done with less duplication. |
Curious: does npm even work correctly on AIX? |
@@ -657,6 +657,10 @@ def configure_node(o): | |||
elif target_arch in ('mips', 'mipsel'): | |||
configure_mips(o) | |||
|
|||
if flavor in ('aix'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what kind of python it is, but this should be if flavor in ('aix',):
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very true. I would say change it to if flavor == 'aix'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd followed the style in the file just a few lines down, but given that we only expect one in the set the == 'aix' is probably better, which change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@Fishrock123 in response to the "does npm work for AIX" the answer is yes given the full set of changes we have in the IBM releases which is what I'm starting to contribute. There are some modules that use binaries which are platform specific but otherwise native modules can be installed/compiled. |
@bnoordhuis the idea of modifying to split for all platforms makes sense to me but I did not want to presume we'd do this to accommodate AIX. |
It seems cleaner than the current approach so I don't think anyone will stop you. |
Thanks for all the good feedback, sorry I'm not had time to update yet but I hope to get to it in the next few days. |
Just testing updates to address most of the comments will post tomorrow if there are no issues. I've looked a bit at changing so all platforms build the lib and then the executable but I have a few concerns:
@bnoordhuis any thoughts ? |
The CI would catch that, wouldn't it? Maybe I misunderstand what you're worried about. That said, while I think the two phase approach would be a little neater due to less duplication, it's not like your current approach is abjectly terrible. Go with whatever you're comfortable with. |
Ok, at this point I think I'd like to stick with what we have instead of changing the other platforms. We want to make progress on nodejs/roadmap#9 so I think we can revisit as part of that. I've pushed an update to address the comments and the tests pass my local CI run, @rvagg, @bnoordhuis, @S-YOU, @mscdex, @thefourtheye can you take another look. |
@@ -160,6 +160,10 @@ def headers(action): | |||
'src/node_version.h', | |||
], 'include/node/') | |||
|
|||
# Add the expfile that is created on AIX | |||
if 'aix' in sys.platform: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry again, but may be startswith
like this is better than in
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@S-YOU updated to reflect your comment. |
find $1 -name "*.a" \ | ||
| xargs nm -Xany -BCpg \ | ||
| awk '{ | ||
if ((($2 == "T") || ($2 == "D") || ($2 == "B")) && (substr($3,1,1) != ".")) { print $3 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: can you break up this line?
LGTM with nit. |
Updated to address Ben's comment and include one small change we found was needed as part of our internal testing. Given Ben's lgtm I'll plan to pull in, in the next day or so |
CI run in advance of landing - https://ci.nodejs.org/job/node-test-pull-request/579/ |
This PR is the first step enabling support for native modules for AIX. The main issue is that unlike linux where all symbols within the Node executable are available to the shared library for a native module (npm), on AIX the symbols must be explicitly exported. In addition, when the shared library is built it must be linked using a list of the available symbols. This patch covers the changes need to: 1) Export the symbols when building the node executable 2) Generate the file listing the symbols that can be used when building the shared library. For AIX, it breaks the build process into 2 steps. The first builds a static library and then generates a node.exp file which contains the symbols from that library. The second builds the node executable and uses the node.exp file to specify which symbols should be exported. In addition, it save the node.exp file so that it can later be used in the creation of the shared library when building a native module. The following additional steps will be required in dependent projects to fully enable AIX for native modules and are being worked separately: - Updates to node-gyp to use node.exp when creating the shared library for a native module - Fixes to gyp related to copying files as covered in https://codereview.chromium.org/1368133002/patch/1/10001 - Pulling in updated gyp versions to Node and node-gyp - Pulling latest libuv These changes were done to minimize the change to other platforms by working within the existing structure to add the 2 step process for AIX without changing the process for other platforms.
CI does not look to have any related failures Squashed down to 1 commit |
This PR is the first step enabling support for native modules for AIX. The main issue is that unlike linux where all symbols within the Node executable are available to the shared library for a native module (npm), on AIX the symbols must be explicitly exported. In addition, when the shared library is built it must be linked using a list of the available symbols. This patch covers the changes need to: 1) Export the symbols when building the node executable 2) Generate the file listing the symbols that can be used when building the shared library. For AIX, it breaks the build process into 2 steps. The first builds a static library and then generates a node.exp file which contains the symbols from that library. The second builds the node executable and uses the node.exp file to specify which symbols should be exported. In addition, it save the node.exp file so that it can later be used in the creation of the shared library when building a native module. The following additional steps will be required in dependent projects to fully enable AIX for native modules and are being worked separately: - Updates to node-gyp to use node.exp when creating the shared library for a native module - Fixes to gyp related to copying files as covered in https://codereview.chromium.org/1368133002/patch/1/10001 - Pulling in updated gyp versions to Node and node-gyp - Pulling latest libuv These changes were done to minimize the change to other platforms by working within the existing structure to add the 2 step process for AIX without changing the process for other platforms. PR-URL: #3114 Reviewed-By: Ben Noordhuis <[email protected]>
Landed as 15bcbf8 |
This PR is the first step enabling support for native modules for AIX. The main issue is that unlike linux where all symbols within the Node executable are available to the shared library for a native module (npm), on AIX the symbols must be explicitly exported. In addition, when the shared library is built it must be linked using a list of the available symbols. This patch covers the changes need to: 1) Export the symbols when building the node executable 2) Generate the file listing the symbols that can be used when building the shared library. For AIX, it breaks the build process into 2 steps. The first builds a static library and then generates a node.exp file which contains the symbols from that library. The second builds the node executable and uses the node.exp file to specify which symbols should be exported. In addition, it save the node.exp file so that it can later be used in the creation of the shared library when building a native module. The following additional steps will be required in dependent projects to fully enable AIX for native modules and are being worked separately: - Updates to node-gyp to use node.exp when creating the shared library for a native module - Fixes to gyp related to copying files as covered in https://codereview.chromium.org/1368133002/patch/1/10001 - Pulling in updated gyp versions to Node and node-gyp - Pulling latest libuv These changes were done to minimize the change to other platforms by working within the existing structure to add the 2 step process for AIX without changing the process for other platforms. PR-URL: #3114 Reviewed-By: Ben Noordhuis <[email protected]>
This PR is the first step enabling support for native modules for AIX. The main issue is that unlike linux where all symbols within the Node executable are available to the shared library for a native module (npm), on AIX the symbols must be explicitly exported. In addition, when the shared library is built it must be linked using a list of the available symbols. This patch covers the changes need to: 1) Export the symbols when building the node executable 2) Generate the file listing the symbols that can be used when building the shared library. For AIX, it breaks the build process into 2 steps. The first builds a static library and then generates a node.exp file which contains the symbols from that library. The second builds the node executable and uses the node.exp file to specify which symbols should be exported. In addition, it save the node.exp file so that it can later be used in the creation of the shared library when building a native module. The following additional steps will be required in dependent projects to fully enable AIX for native modules and are being worked separately: - Updates to node-gyp to use node.exp when creating the shared library for a native module - Fixes to gyp related to copying files as covered in https://codereview.chromium.org/1368133002/patch/1/10001 - Pulling in updated gyp versions to Node and node-gyp - Pulling latest libuv These changes were done to minimize the change to other platforms by working within the existing structure to add the 2 step process for AIX without changing the process for other platforms. PR-URL: #3114 Reviewed-By: Ben Noordhuis <[email protected]>
This PR is the first step enabling support for native modules for AIX. The main issue is that unlike linux where all symbols within the Node executable are available to the shared library for a native module (npm), on AIX the symbols must be explicitly exported. In addition, when the shared library is built it must be linked using a list of the available symbols. This patch covers the changes need to: 1) Export the symbols when building the node executable 2) Generate the file listing the symbols that can be used when building the shared library. For AIX, it breaks the build process into 2 steps. The first builds a static library and then generates a node.exp file which contains the symbols from that library. The second builds the node executable and uses the node.exp file to specify which symbols should be exported. In addition, it save the node.exp file so that it can later be used in the creation of the shared library when building a native module. The following additional steps will be required in dependent projects to fully enable AIX for native modules and are being worked separately: - Updates to node-gyp to use node.exp when creating the shared library for a native module - Fixes to gyp related to copying files as covered in https://codereview.chromium.org/1368133002/patch/1/10001 - Pulling in updated gyp versions to Node and node-gyp - Pulling latest libuv These changes were done to minimize the change to other platforms by working within the existing structure to add the 2 step process for AIX without changing the process for other platforms. PR-URL: #3114 Reviewed-By: Ben Noordhuis <[email protected]>
This PR is the first step enabling support for native modules
for AIX. The main issue is that unlike linux where all
symbols within the Node executable are available to the shared
library for a native module (npm), on AIX the symbols must
be explicitly exported. In addition, when the shared library is
built it must be linked using a list of the available symbols.
This patch covers the changes need to:
building the shared library.
For AIX, it breaks the build process into 2 steps. The first builds
a static library and then generates a node.exp file which contains
the symbols from that library. The second builds the node executable
and uses the node.exp file to specify which symbols should be
exported. In addition, it save the node.exp file so that it can
later be used in the creation of the shared library when building
a native module.
The following additional steps will be required in dependent projects
to fully enable AIX for native modules and are being worked
separately:
shared library for a native module
https://codereview.chromium.org/1368133002/patch/1/10001
These changes were done to minimize the change to other platforms
by working within the existing structure to add the 2 step process
for AIX without changing the process for other platforms.