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

Zos fixes #1451

Closed
wants to merge 2 commits into from
Closed

Zos fixes #1451

wants to merge 2 commits into from

Conversation

jBarz
Copy link
Contributor

@jBarz jBarz commented May 30, 2018

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

On z/OS, we fixed up node-gyp that comes packaged with node 6.x.
This PR transports those fixes here so that node-gyp works as a standalone.

@jBarz jBarz changed the title Zos Zos fixes May 30, 2018
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.

lib/configure.js Outdated
'node',
'out/Release/obj.target/libnode',
'out/Debug/obj.target/libnode',
'lib/libnode'].map(function(file) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be an idea to rewrite the comment just prior to this block as it's no longer AIX specific. We can probably drop the candidate locations from the comment as it's a duplicate of the code. Something like:

// For AIX and z/OS we need to set up the path to the exports file
// which contains the symbols needed for linking. 

Also will the exports file ever be include/node/node.x on z/OS? If not, we could slightly speed up by using a different set of candidates per platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @richardlau
I will rewrite that comment.
Also the exports file on z/OS will never be include/node/node.x
So I will do as you suggested.

@richardlau
Copy link
Member

jBarz added 2 commits May 30, 2018 15:16
NodeJS on z/OS uses shared dll (libnode.so). When linking native
addons, node-gyp needs to find the corresponding libnode.x during
the link step.
This reverts commit 40edac20c8c9644067636e5722c389954890bccf.
@jBarz
Copy link
Contributor Author

jBarz commented May 30, 2018

new CI after I addressed nits.

@refack
Copy link
Contributor

refack commented May 30, 2018

@@ -233,7 +233,12 @@ def LoadOneBuildFile(build_file_path, data, aux_data, includes,
# Open the build file for read ('r') with universal-newlines mode ('U')
# to make sure platform specific newlines ('\r\n' or '\r') are converted to '\n'
# which otherwise will fail eval()
build_file_contents = open(build_file_path, 'rU').read()
if sys.platform == 'zos':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try to upstream it. Or in other words, will it be a problem for node (if we have non ASCII chars, possibly in config.gypi via node-prefix for example) ?

Or will it be better that node-gyp will generate UTF-8-BOM files, and skip patching GYP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack I tried looking for what node-prefix does. but couldn't find anything. Can you point me to some documentation about it?

bnoordhuis pushed a commit that referenced this pull request Jun 8, 2018
Node.js on z/OS uses shared dll (libnode.so). When linking native
addons, node-gyp needs to find the corresponding libnode.x during
the link step.

PR-URL: #1451
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
bnoordhuis pushed a commit that referenced this pull request Jun 8, 2018
PR-URL: #1451
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@bnoordhuis
Copy link
Member

Cheers, landed in 293092c...eaedc12.

@bnoordhuis bnoordhuis closed this Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants