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

Absolute Smiles JNI-inchi fix #798

Closed
wants to merge 3 commits into from

Conversation

dkatzel-ncats
Copy link
Contributor

This is the fix for #797 as well as adding the fix for 2.7.1 to the 2.8 branch.

This is as small a change as possible. Alternative approaches are to make the inchi options and return value Strings instead of enums so there wouldn't be a dependency for either JNA or JNI inchi. but there would then be a lot more String parsing into those enums every time which would might lead to performance problems.

Another approach would be to make a CDK inchi interface that wraps the underlying JNI/JNA implementation but that would be more code changes and API breaking changes.

@egonw egonw requested a review from johnmay January 11, 2022 20:49
@egonw
Copy link
Member

egonw commented Jan 11, 2022

I think the idea is to fix this instead by (runtime?) depending on the new cdk-jniinchi-support module, @johnmay?

@dkatzel-ncats
Copy link
Contributor Author

yeah I wasn't sure if the idea going forward was to assume JNA-inchi and have additional support for jni so things should default to jna or if you wanted to make it more flexible based on which modules are loaded?

@egonw
Copy link
Member

egonw commented Jan 11, 2022

@dkatzel-ncats, can you try the current master (so, without this patch), but add this to your pom.xml:

    <dependency>
      <groupId>org.openscience.cdk</groupId>
      <artifactId>cdk-jniinchi-support</artifactId>
      <version>2.7.1</version>
    </dependency>

Does that solve it?

@dkatzel-ncats
Copy link
Contributor Author

That would fix the compiler error but my downstream software can't use JNI inchi we must use JNA

@egonw
Copy link
Member

egonw commented Jan 11, 2022

No, I think this cdk-jniinchi-support module just provides the two classes, but not actually use the JNI version... checking!

@egonw
Copy link
Member

egonw commented Jan 11, 2022

@dkatzel-ncats, check https://github.com/cdk/cdk/tree/master/storage/jniinchi-support/src/main/java/net/sf/jniinchi and if you like that module's pom file too... I think it's really just these two classes.

@egonw egonw removed the request for review from johnmay January 11, 2022 21:01
@johnmay
Copy link
Member

johnmay commented Jan 11, 2022

Yep include the jniinchi-support-module for now, not doing another release sorry am swamped ATM.

@johnmay
Copy link
Member

johnmay commented Jan 11, 2022

I'll fix on master though, it's just a mater of not using the INCHI_OPTION etc

@johnmay johnmay closed this Jan 11, 2022
@johnmay
Copy link
Member

johnmay commented Jan 11, 2022

To clarify as Egon said it's jus the Enum's not the actual JNI stuff. It's basically to provide backwards compatible API support, everything still uses JNA inchi.

@dkatzel-ncats
Copy link
Contributor Author

I can confirm using 2.7.1 with jniinchi-support to my software that uses cdk does make everything work using 2.7.1 thanks!

@johnmay
Copy link
Member

johnmay commented Jan 11, 2022

Fix on master once pulled, sorry about that - experimenting with optional dependencies and so they are a little magical such that even the tests didn't pick this up. Please let us know if you find other issues.

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

3 participants