search

NeuronRobotics / nrjavaserial

A Java Serial Port system. This is a fork of the RXTX project that uses in jar loading of the native code.
Other
345 stars 143 forks source link

Make some imports optional #100

Closed crea-doo closed 7 years ago

crea-doo commented 7 years ago

Try to solve an issue mentioned in https://github.com/NeuronRobotics/nrjavaserial/pull/98 where the generated MANIFEST.MF has mandatory package-imports for com.sun.jna.platform.win32and org.apache.commons.net.telnet. Both of them should be optional.

msteigenberger commented 7 years ago

Hi, I think "com.sun.jna.platform.win32" is not optional. It's used by RXTXCommDriver that is used to obtain the available serial ports. -> It would definitely cause a CNF.

crea-doo commented 7 years ago

Thx for the feedback - I've also read your comments in https://github.com/openhab/nrjavaserial/pull/1. So obviously it would be better to make this dependency not optional. I'll make the necessary changes soon...

andyrozman commented 7 years ago

I think we switched to jna now, from last release to this one, so jna is not optional. But telnet support is... I never supply that library with my product (which uses nrjavaserial), because I use nrjavaserial only for standard serial communication... So if you want to run without jna, you would need to use older version of nrjavaserial.

Andy

On Sat, Aug 19, 2017 at 1:23 AM crea-doo notifications@github.com wrote:

Thx for the feedback - I've also read your comments in openhab#1 https://github.com/openhab/nrjavaserial/pull/1. So obviously it would be better to make this dependency not optional. I'll make the necessary changes soon...

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NeuronRobotics/nrjavaserial/pull/100#issuecomment-323487313, or mute the thread https://github.com/notifications/unsubscribe-auth/AFPs5c1X8aTN96EP4SPwAUr6HfmiO0tNks5sZirsgaJpZM4Nm7-p .

msteigenberger commented 7 years ago

IMHO also telnet is not optional as it would cause a CNF when using rfc2217 which is included in this library. To solve that, rfc2217 packages would need to be extracted to a separate module. But maybe this effort is too high for just that edge case.

@crea-doo: thanks for solving

kaikreuzer commented 7 years ago

I think "com.sun.jna.platform.win32" is not optional.

@msteigenberger It seems to me that you do not fully understand the idea of optional imports in OSGi. Look at it that way: Do you really believe xxxx.win32 is mandatory on a Linux or Mac system? If your answer is no, the import hence has to be optional - and that is the change that has been correctly done in this PR.

IMHO also telnet is not optional as it would cause a CNF when using rfc2217 which is included in this library.

Same logic here: Is the telnet dependency required for people like @andyrozman who are NOT using rfc2217? The clear answer it no and therefore also this dependency has to be optional, as it has been correctly set in this PR.

msteigenberger commented 7 years ago

@kaikreuzer I do understand optional imports in OSGI. I just think it's not a clean solution to make something optional that is not optional in a specific branch. Do you think it is good that something can be resolved at compile time but is then not working on specific systems (ln this case windows)? Or you would get CNF when using specific functions of the library (rfc2217)?

To answer your first question: I was wrong when saying it causes CNF at any time. It's just a java import statement but the branch would not be executed on a Linux/Mac system -> I correct my answer to NO, it is not mandatory on Linux/Mac system (in contrast what I thought initially).

Second question (telnet dependency): Also this is not necessary for all people.

Usually needing optional imports means poor cohesion and this the case here. But I would say it's also not worth the effort to split the library in specific modules.

-> Said that, I would agree leaving both packages as optional. Just wanted to tell you my concerns in this ;-).

crea-doo commented 7 years ago

Taking into account the arguments/questions from @kaikreuzer, I would leave the code like it is.

I had a look that the JNA dependency was introduced in #85 only to have access to the Windows registry to faster resolve the available ports on Windows rather than a switch from JNI to JNA like @andyrozman assumes.

If we want it to be functional on Windows even if the JNA dependencies are not present, we should add a check for JNA and as a fallback leave the old behaviour, where all ports are checked one after the other.

In terms of rfc2217 support one would get a CNF on every platform (and not only on a specific), so I also would leave it as it is now.

msteigenberger commented 7 years ago

@crea-doo your suggestion would make JNA as a real optional dependency (would always work without JNA) and is IMHO therefore the best!

crea-doo commented 7 years ago

I restored the branch and made the necessary changes (see https://github.com/crea-doo/nrjavaserial/tree/osgi-imports) I don't know if the changes on an already merged branch can be merged. If not I'll create another pull request...

kaikreuzer commented 7 years ago

I don't know if the changes on an already merged branch can be merged.

No, it can't - you will need to create a new PR.