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: support platform #1276

Merged
merged 2 commits into from
Sep 26, 2017
Merged

zos: support platform #1276

merged 2 commits into from
Sep 26, 2017

Conversation

jBarz
Copy link
Contributor

@jBarz jBarz commented Aug 24, 2017

  • recognize a zos system
  • use correct cp arguments for zos
  • add zos compile options
  • add zos makedep arguments
  • use export file in link step on zos
Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

@nodejs/build ... thoughts on this?

@richardlau
Copy link
Member

Are the gyp changes being upstreamed?

@jBarz
Copy link
Contributor Author

jBarz commented Aug 25, 2017

Yes. I am in the process of doing that.

@refack
Copy link
Contributor

refack commented Aug 25, 2017

Are the gyp changes being upstreamed?

Yes. I am in the process of doing that.

I don't think that upstream GYP has a zos try-bot, but at least we could get some regression testing.

@@ -429,6 +429,8 @@ def GetFlavor(params):
return 'netbsd'
if sys.platform.startswith('aix'):
return 'aix'
if sys.platform.startswith('os390'):
return 'os390'
Copy link
Contributor

@refack refack Aug 25, 2017

Choose a reason for hiding this comment

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

if the platform is named zos why use os390?

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure that's what uname -m prints on z/OS machines.

Copy link
Contributor

@refack refack Aug 26, 2017

Choose a reason for hiding this comment

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

Ack. So that's the input (L432), I'm wondering is we should keep it as the internal alias?
We could do something similar to:

if sys.platform.startswith('sunos'):
return 'solaris'

if that's more congruent with the IBM's future plans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of z/OS projects built using gyp already expect 'os390'. So that would require a major change across multiple z/OS projects.

Copy link
Contributor

@refack refack Aug 28, 2017

Choose a reason for hiding this comment

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

I meant the other way around, i.e.:

   if sys.platform.startswith('os390'): 
     return 'zos'

since the return os390 (a.k.a. flavor) is only used as an internal alias to map to

elif flavor == 'os390':

But in anycase, you IBM people know best. I'm just asking...

Copy link
Member

@gibfahn gibfahn Aug 28, 2017

Choose a reason for hiding this comment

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

@jBarz we could just alias it inside the node build scripts I guess.

However I think os390 is the standard "technical" name for the ARCH, like x86_64 is for Intel, so it's probably be less surprising to stick with that.

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 apologies, you're right that this is only internal alias.
@gibfahn is right though, 'os390' is a standard technical name for z/OS arch. So I would prefer to leave it as 'os390' if that is ok with you

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Approving as a floating patch since IMHO it's not going to be a trivial to upstream.

@refack
Copy link
Contributor

refack commented Aug 28, 2017

@gibfahn
Copy link
Member

gibfahn commented Aug 28, 2017

Approving as a floating path since IMHO it's not going to be a trivial to upstream.

@jBarz you mentioned that other z/OS projects are using GYP, are there plans to upstream the gyp related changes?

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Need to fix some python syntax

@@ -2250,7 +2250,10 @@ def GenerateOutputForConfig(target_list, target_dicts, data, params,
master_ninja.rule(
'copy',
description='COPY $in $out',
command='rm -rf $out && cp -af $in $out')
if sys.platform in ('os390'):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a python syntax error:

gyp info spawn args   '-Goutput_dir=.' ]
Traceback (most recent call last):
  File "c:\workspace\nodegyp-test-commit\nodes\win2012r2\gyp\gyp_main.py", line 16, in <module>
    sys.exit(gyp.script_main())
  File "c:\workspace\nodegyp-test-commit\nodes\win2012r2\gyp\pylib\gyp\__init__.py", line 545, in script_main
    return main(sys.argv[1:])
  File "c:\workspace\nodegyp-test-commit\nodes\win2012r2\gyp\pylib\gyp\__init__.py", line 538, in main
    return gyp_main(args)
  File "c:\workspace\nodegyp-test-commit\nodes\win2012r2\gyp\pylib\gyp\__init__.py", line 514, in gyp_main
    options.duplicate_basename_check)
  File "c:\workspace\nodegyp-test-commit\nodes\win2012r2\gyp\pylib\gyp\__init__.py", line 91, in Load
    generator = __import__(generator_name, globals(), locals(), generator_name)
  File "c:\workspace\nodegyp-test-commit\nodes\win2012r2\gyp\pylib\gyp\generator\msvs.py", line 15, in <module>
    import gyp.generator.ninja as ninja_generator
  File "c:\workspace\nodegyp-test-commit\nodes\win2012r2\gyp\pylib\gyp\generator\ninja.py", line 2253
    if sys.platform in ('os390'):
     ^
SyntaxError: invalid syntax

You could do the if before hand near L2167, or use the python ternary

lib/configure.js Outdated
@@ -270,6 +270,23 @@ function configure (gyp, argv, callback) {
return callback(new Error(msg))
}
}
else if (process.platform === 'os390') {
Copy link
Contributor

Choose a reason for hiding this comment

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

usually else if should be on the same line as the previous }

lib/configure.js Outdated
fs.accessSync(node_exp_file, fs.R_OK)
// exp file found, stop looking
break
} catch (exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should rename exception to _ to indicate it's intentionally ignored

@@ -4,6 +4,9 @@
reload(sys)

def main():
if sys.platform.startswith('os390'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you replace this with

if not encoding:
  return False

after L10

@jBarz
Copy link
Contributor Author

jBarz commented Aug 29, 2017

Fixed issues.

@jBarz
Copy link
Contributor Author

jBarz commented Aug 29, 2017

@jBarz you mentioned that other z/OS projects are using GYP, are there plans to upstream the gyp related changes?

Yes, these changes will go upstream.

@refack
Copy link
Contributor

refack commented Aug 29, 2017

Fixed issues.

Thanks for following up.
Let's see what the CI sais: https://ci.nodejs.org/job/nodegyp-test-pull-request/36/ ✔️

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM

@refack
Copy link
Contributor

refack commented Aug 29, 2017

@jBarz @gibfahn is what time frame would you like to see this out? Since Ben is on vacation, and I'd like for him to take a look.

@jBarz
Copy link
Contributor Author

jBarz commented Aug 29, 2017

No urgency :-). We can wait

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.

LGTM with a question.

lib/configure.js Outdated
for (var next = 0; next < candidates.length; next++) {
node_exp_file = path.resolve(node_root_dir, candidates[next])
try {
fs.accessSync(node_exp_file, fs.R_OK)
Copy link
Member

Choose a reason for hiding this comment

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

fs.accessSync() does not exist in v0.10 and v0.12 (see also #955) but since this is a z/OS-only code path, I guess that's alright. That said, can't this be merged with the AIX code path above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it can be merged with AIX code. Will do

@jBarz
Copy link
Contributor Author

jBarz commented Aug 29, 2017

Merged the AIX and z/OS code paths to find the export file

@jBarz
Copy link
Contributor Author

jBarz commented Aug 31, 2017

@refack
Copy link
Contributor

refack commented Aug 31, 2017

Wow Dirk was quick :)
I'll incorporate into #1183

@saper
Copy link
Contributor

saper commented Sep 2, 2017

@jBarz Is there any way a mere mortal can try this? For example using Hercules? (I only have ancient MVS running there).

lib/configure.js Outdated
var candidates = ['include/node/node',
'out/Release/node',
'out/Debug/node',
'node'].map((file) => {
Copy link
Member

Choose a reason for hiding this comment

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

You can't use arrow functions just yet, they don't parse in old node.js versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

lib/configure.js Outdated
var logprefix = 'find exports file'
node_exp_file = findAccessibleSync(logprefix, node_root_dir, candidates)
if (node_exp_file !== undefined) {
log.verbose(logprefix, 'Found exports file: %s', node_exp_file)
} else {
var msg = msgFormat('Could not find node.exp file in %s', node_root_dir)
var msg = msgFormat(`Could not find node.${ext} file in %s`, node_root_dir)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto: no backticks just yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

lib/configure.js Outdated
'out/Release/node',
'out/Debug/node',
'node'].map(function(file) {
return `${file}.${ext}`
Copy link
Member

Choose a reason for hiding this comment

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

No backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, i missed that one!

Copy link
Contributor

Choose a reason for hiding this comment

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

Why no backticks? node-gyp@4 is node>=4.

node-gyp/package.json

Lines 38 to 40 in 7245415

"engines": {
"node": ">= 4.0.0"
},

Copy link
Member

Choose a reason for hiding this comment

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

Well, that was a surreptitious change. I wouldn't have hidden that in an otherwise unrelated commit. (I'm talking about 5f924ce.)

At any rate, this way we can do another 3.x release and include this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, no harm in keeping compatibility.

lib/configure.js Outdated
'out/Release/node',
'out/Debug/node',
'node'].map(function(file) {
return '' + file + '.' + ext
Copy link
Member

Choose a reason for hiding this comment

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

First (empty) concatenation isn't necessary but LGTM apart from that.

* recognize a zos system
* use correct cp arguments for zos
* add zos compile options
* add zos makedep arguments
* use export file in link step on zos
@refack
Copy link
Contributor

refack commented Sep 7, 2017

@jBarz
Copy link
Contributor Author

jBarz commented Sep 12, 2017

@refack Do you know approx when this could be landed and released?
I just wanted an estimate of the date for node-gyp to support z/OS.
z/OS support for nodejs was released this month so I suspect some of our users will run into this issue :-).
And the next release version is 4.0.0, correct?

@gibfahn
Copy link
Member

gibfahn commented Sep 26, 2017

I think this is good to go, given that the GYP changes have been upstreamed.

CI: https://ci.nodejs.org/view/Node.js/job/nodegyp-test-pull-request/40/

EDIT: CI is green.

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.

7 participants