-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
unix,win: add uv_os_{get,set}priority() #1945
Conversation
test/test-process-priority.c
Outdated
|
||
TEST_IMPL(process_priority) { | ||
#if defined(__MVS__) | ||
RETURN_SKIP("functionality not supported on zOS"); |
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.
There's nothing in the docs/implementation of uv_os_{get,set}priority()
mentioning this -- On what basis is the functionality not supported on z/OS?
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.
It was returning ENOTSUP.
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.
Assuming uv_os_{get,set}priority()
is not stubbed out on z/OS and this is actually being returned by the underlying system call (setpriority
?), would it not be better to skip the test based on the error returned instead of assuming the functionality is not supported on all z/OS?
cc @jBarz @mmallick-ca @john-yan (I'm not sure who's in @libuv/zos but I can't ping them directly since I'm not in the libuv org).
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.
The function is indeed being called on z/OS but returning ENOSYS.
From the documentation:
"If the ENOSYS return code is received, your installation does not support this service. Contact your system administrator if you require activation of this service."
So as @richardlau suggests, try setting priority "0", and if that returns ENOTSUP, then skip test?
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.
LGTM. Wasn't there an issue somewhere where this was discussed?
On Windows, the returned priority will equal one of the `UV_PRIORITY` | ||
constants. | ||
|
||
.. versionadded:: REPLACEME |
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.
Good idea. Should we add smthg to https://github.com/libuv/libuv-release-tool?
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.
|
ec9931c
to
eff866f
Compare
Updated to try to test on zOS. CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/993/ |
Refs: nodejs/node#21675 PR-URL: libuv#1945 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Refs: nodejs/node#21675 PR-URL: #1945 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Force pushed with lease to replace |
No description provided.