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

Adds a Java accessor for GetVersionString #14876

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

Craigacp
Copy link
Contributor

@Craigacp Craigacp commented Mar 2, 2023

Description

Java part of #14873.

version = initialiseVersion();
if (version == null) {
throw new IllegalStateException("Failed to load version string from native library");
}
Copy link
Member

Choose a reason for hiding this comment

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

This should not generally fail, it does not even return an error code. However, I feel that running this and caching it and risking potential failure, even though most of the people do not need it, is not practical. We only had 1 GH issue that requested it.

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's there to catch the native code returning null, but if that can't happen I'll remove this.

(void)clazz; // required JNI parameter not needed by functions which don't access their host class.
const char* version = OrtGetApiBase()->GetVersionString();
jstring versionStr = NULL;
if (version != NULL) {
Copy link
Member

@yuslepukhin yuslepukhin Mar 2, 2023

Choose a reason for hiding this comment

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

if (version != NULL) {

This will neve be null. Redundant check. assert() might be more appropriate, meaning something is wrong with the build. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll remove that and see if the asserts are available from there.

Copy link
Member

@yuslepukhin yuslepukhin left a comment

Choose a reason for hiding this comment

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

🕐

@Craigacp
Copy link
Contributor Author

Craigacp commented Mar 3, 2023

I've updated it to simplify things assuming that GetVersionString never returns null. I tried to use the idiom suggested here for the unused parameters, but my IDE and clang think that's a C2x extension which isn't finalized yet, so it uses the old idiom.

Copy link
Member

@yuslepukhin yuslepukhin left a comment

Choose a reason for hiding this comment

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

:shipit:

@yuslepukhin
Copy link
Member

/azp run MacOS CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-python-checks-ci-pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@yuslepukhin
Copy link
Member

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@yuslepukhin
Copy link
Member

/azp run orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-binary-size-checks-ci-pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@yuslepukhin yuslepukhin merged commit 150043f into microsoft:main Mar 7, 2023
mszhanyi pushed a commit that referenced this pull request Mar 9, 2023
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.

2 participants