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.

Sign up for GitHub

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

Jump to bottom

aix: fix os.release() #10245

Closed
wants to merge 3 commits into from
Closed

aix: fix os.release() #10245

wants to merge 3 commits into from

Conversation

jBarz
Copy link
Contributor

@jBarz jBarz commented Dec 13, 2016 •
edited
Loading

Checklist
  • make -j4 test (UNIX)
Affected core subsystem(s)

os.release() on aix

Description of change

On AIX info.release is only a part of OS release version.
We need to combine info.version and info.release

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v4.x labels Dec 13, 2016
@jBarz jBarz mentioned this pull request Dec 13, 2016
Closed
richardlau
richardlau reviewed Dec 13, 2016
View reviewed changes
src/node_os.cc
@@ -85,7 +85,13 @@ static void GetOSRelease(const FunctionCallbackInfo<Value>& args) {
if (uname(&info) < 0) {
return env->ThrowErrnoException(errno, "uname");
}
# ifdef _AIX
char release[256];
sprintf(release, "%s.%s", info.version, info.release);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use snprintf for consistency with the rest of this file?

@mscdex mscdex added aix Issues and PRs related to the AIX platform. os Issues and PRs related to the os subsystem. labels Dec 13, 2016
@Trott
Copy link
Member

Trott commented Dec 13, 2016

Should we add a test for this? Currently in test/parallel/test-os.js, there's this:

const release = os.release();
console.log('release = ', release);
is.string(release);
assert.ok(release.length > 0);

Maybe add something like this right below it?

//TODO: Check format on more than just AIX
if (common.isAix)
  assert.ok(/^\d+\.\d+\.\d+$/.test(release)); // <-- substitute whatever regexp makes sense for AIX

@Trott
Copy link
Member

Trott commented Dec 13, 2016 •
edited
Loading

The docs for os.release() say this:

On POSIX systems, the operating system release is determined by calling uname(3).

With this change, is that still accurate for AIX? (If so, then great. If not, then perhaps a doc update should be included.)

@gibfahn
Copy link
Member

gibfahn commented Dec 13, 2016

cc/ @nodejs/platform-aix

@richardlau
Copy link
Member

The docs for os.release() say this:

On POSIX systems, the operating system release is determined by calling uname(3).

With this change, is that still accurate for AIX? (If so, then great. If not, then perhaps a doc update should be included.)

Yes, the change is still accurate -- The code is still calling uname but is now combining two fields instead of just returning one. On AIX it looks like uname splits the OS version over two struct utsname fields, e.g. AIX 7.1 returns 7 in the version field and 1 in the release field.

mhdawson
mhdawson approved these changes Dec 13, 2016
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM. Would be nice to fill in additional test for other platforms, but since this is broken on AIX, don't want to hold up landing this for that.

@mhdawson
Copy link
Member

@Trott look ok to you now ?

@Trott
Copy link
Member

Trott commented Dec 14, 2016

@mhdawson Test looks good to me. (The C++ change seems good to me too but I'm a beginner-at-best C++ coder so that probably doesn't mean much.)

@jasnell
Copy link
Member

jasnell commented Dec 23, 2016

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

@jasnell jasnell self-assigned this Dec 23, 2016
jasnell pushed a commit that referenced this pull request Dec 27, 2016
@jBarz @jasnell
os: fix os.release() for aix and add test
1c3c75d 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Dec 27, 2016

Landed in 1c3c75d

@jasnell jasnell closed this Dec 27, 2016
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@jBarz @evanlucas
os: fix os.release() for aix and add test
899496a 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@jBarz @evanlucas
os: fix os.release() for aix and add test
75efdeb 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@jBarz @MylesBorins
os: fix os.release() for aix and add test
e1cd70e 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@jBarz @MylesBorins
os: fix os.release() for aix and add test
67c8885 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@jBarz @MylesBorins
os: fix os.release() for aix and add test
ff5c111 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@jBarz @MylesBorins
os: fix os.release() for aix and add test
7708071 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@jBarz @MylesBorins
os: fix os.release() for aix and add test
adb5bed 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Jan 24, 2017
Merged
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@jBarz @MylesBorins
os: fix os.release() for aix and add test
3eb9373 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@jBarz @MylesBorins
os: fix os.release() for aix and add test
7505b86 
PR-URL: #10245
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Feb 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees

@jasnell jasnell

Labels
aix Issues and PRs related to the AIX platform. c++ Issues and PRs that require attention from people who are familiar with C++. os Issues and PRs related to the os subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@jBarz @Trott @gibfahn @richardlau @mhdawson @jasnell @cjihrig @mscdex @MylesBorins @nodejs-github-bot