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

unix,win: add uv_os_{get,set}priority() #1945

Merged
merged 1 commit into from
Aug 15, 2018
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 13, 2018

No description provided.


TEST_IMPL(process_priority) {
#if defined(__MVS__)
RETURN_SKIP("functionality not supported on zOS");
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was returning ENOTSUP.

Copy link
Contributor

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).

Copy link
Contributor

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?

Copy link
Member

@santigimeno santigimeno left a 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
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richardlau
Copy link
Contributor

LGTM. Wasn't there an issue somewhere where this was discussed?

nodejs/node#21675

@cjihrig cjihrig force-pushed the setpriority branch 2 times, most recently from ec9931c to eff866f Compare August 14, 2018 14:22
@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 14, 2018

Updated to try to test on zOS.

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/993/
The only failures on win2012r2-vs2017 and alpine-last-latest-x64 are unrelated.

Refs: nodejs/node#21675
PR-URL: libuv#1945
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@cjihrig cjihrig merged commit 81c7742 into libuv:v1.x Aug 15, 2018
@cjihrig cjihrig deleted the setpriority branch August 15, 2018 13:01
cjihrig added a commit that referenced this pull request Aug 15, 2018
Refs: nodejs/node#21675
PR-URL: #1945
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 15, 2018

Force pushed with lease to replace REPLACEME with 1.23.0. New commit is e57e071.

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.

None yet

4 participants