Skip to content

Minor Code / Dependencies Updates #537

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 12 commits into from
Closed

Minor Code / Dependencies Updates #537

wants to merge 12 commits into from

Conversation

@jvmahon
Copy link

@jvmahon jvmahon commented Dec 23, 2017

Minor code update to allow versions of node-persist >=1.0.0 to be used which altered the return syntax of GetItem. Updated other dependencies to recent versions.

KhaosT
KhaosT reviewed Dec 25, 2017
View reviewed changes
package.json Outdated
"debug": "^2.2.0",
"ed25519": "^0.0.4",
"debug": "^3.1.0",
"ed25519": "git://github.com/jvmahon/ed25519",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why pin a git repo instead of the npm package?

KhaosT
KhaosT reviewed Dec 25, 2017
View reviewed changes
package.json Outdated
{
"name": "hap-nodejs",
"version": "0.4.36",
"version": "0.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't bump version number. It will be coordinated in release.

KhaosT
KhaosT reviewed Dec 25, 2017
View reviewed changes
package.json
"decimal.js": "^7.2.3"
"ip": "^1.1.5",
"mdns": "^2.3.4",
"node-persist": "^2.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does node-persist backwards compatible with data created in older version?

@jvmahon
Copy link
Author

jvmahon commented Dec 25, 2017 via email

@jvmahon
Copy link
Author

jvmahon commented Dec 25, 2017 via email

@jvmahon
Copy link
Author

jvmahon commented Dec 25, 2017 via email

@KhaosT
Copy link
Contributor

KhaosT commented Jan 6, 2018

Thanks, I'll try to test it this weekend :) Sorry for the delay.

@jvmahon
Copy link
Author

jvmahon commented Jan 6, 2018

Thanks. As a practical matter, this change doesn't add new features or correct any bugs that I'm aware of - its more on the order of allowing the more updated node-persist and its features just in case others want to make improvements based on that. So its more of a "for consideration in your future development" kind of thing.

On a more relevant note, I've also been working on an update to the ed25519 module which does fix some issues. Specifically, the update gets rid of the many dozens of compiler warnings you get from the ed25519 module on Windows 64-bit (and possibly others?) during compile when installing hap-nodejs. The updated module simply uses the orlp/ed25519 updated ed25519 code base which has more explicit type casting. I have submitted this as a PR to the dozoe/ed25519 though its unclear if that repository is still getting updates. If it doesn't get incorported there, I'll put it on NPM. I mention this because if you are doing further testing of the hap-nodejs, you may want to consider testing it with the updated ed25519. Its at https://github.com/jvmahon/ed25519 .

Thanks for all your work on this software - its been a huge help to me and I'm sure to many others!

As an FYI - I've also put a detailed Windows 64 bit install guide at http://github.com/jvmahon/homebridge in the file "HomeBridge on Windows 64 bit.docx". I'm still working on that. Its currently in Word, but once its finished, I'll try to convert it to MD. I mention this because it may also be helpful to those installing hap-nodejs without homebridge so, eventually, you might consider a link to it if you think it useful to users of your software who might not be also installing HomeBridge. I'll let you know once its finished if you want.

@volschin
Copy link

volschin commented Jan 6, 2018

In my understanding your ed25519 change breaks the compatibility with a lot of existing OpenSSL 1.0.x installations on Windows, also supported as STABLE until 2019. You need to implement a fix, that takes this into regard.

@jvmahon
Copy link
Author

jvmahon commented Jan 6, 2018

volschin - Thanks for the info. The fix was actually intended, at least in part, to fix an incompatibility that arises when OpenSSL versions greater than 1.0.x are already installed on a Windows system by making the ed25519 code work with both pre- and post-1.0 versions of OpenSSL (OpenSSL versions > 1.0 didn't work on windows with the original ed25519 due to a renaming of OpenSSL's libeay32.lib to libcrypto.lib). I understand the explicit linking to the OpenSSL libeay32.lib in ed25519's bindings.gyp is no longer needed on Windows since nodejs includes OpenSSL -- thus, I commented out the explicit library link to libeay32.lib; as a result, the ed25519 code should rely on the OpenSSL linked into nodejs. It seems to work when I test it (at least on Windows 10 64 bit with nodejs 8). Can you point me to any info. discussing the incompatibility? Maybe it can be addressed while still allowing code that doesn't generate dozens of compiler warnings as was previously the case.

@volschin
Copy link

volschin commented Jan 6, 2018

Here is some general discussion about the topic
nodejs/node#13575

Using OpenSSL 1.1 in nodejs will make ed25519 accessible via internal crypto api and an additional module will no longer be needed. But this is open at the moment.

@jvmahon
Copy link
Author

jvmahon commented Jan 6, 2018

volschin - Thanks. I appreciate your insights. I still don't really understand how my mods cause an OpenSSL compatibility break as you've suggested. But maybe the concern can be narrowed. In my mods, there are two categories of changes - (1) the substitution of the orlp/ed25519 code for the original (this is really just about proper type casting and was the more "important" change in my view as it gets rid of dozens of warning errors when compiling on Win-64); and (2) removing the explicit libeay32.lib inclusion from the Windows-OS section in bindings.gyp and, instead, relying on the OpenSSL linked into nodejs. Since change #1 is the more important (to me), I could revert the bindings.gyp changes (change #2) to its original form if that is where the OpenSSL break is that you are discussing (I'm still not sure there is a break, but i the interest of getting the #1 fixes potentially tested, #2 seems discardable).

Since this discussion is really about the ed25519 modification and no longer about the hap-nodejs, if you have further thoughts, I suggest continuing at my repository http://github.com/jvmahon/ed25519 so as not to clutter up this thread too much.

@KhaosT
Copy link
Contributor

KhaosT commented Jan 8, 2018

Okay I did some tests and it looks like the changes in node-persist (1.0 and later) is not backward compatible with the original version we are using and they didn't provide a way to migrate (other than say bundle both version and manually set old data to new one)...
As a result of that, I'm afraid we have to leave node-persist in 0.0.11 for now...

@jvmahon
Copy link
Author

jvmahon commented Jan 10, 2018

KhaosT - Since your testing shows the update to node-persist isn't backward compatible, I think it makes sense for me to close this PR. I plan to close it tomorrow unless you suggest otherwise.

@jvmahon jvmahon closed this Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KhaosT KhaosT KhaosT left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jvmahon @KhaosT @volschin