Skip to content
This repository has been archived by the owner on Feb 6, 2021. It is now read-only.

Specify Encoding for Version String (#87) #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

m4dc4p
Copy link

@m4dc4p m4dc4p commented Jan 19, 2018

Added an annotation to the sodium_version_string() method so it
is always read as an ASCII string. Avoids some issues on Windows
with reading multi-byte strings by default.

Added a test to make sure version string matches the expected
pattern.

For issue #87.

Added an annotation to the sodium_version_string() method so it
is always read as an ASCII string. Avoids some issues on Windows
with reading multi-byte strings by default.

Added a test to make sure version string matches the expected
pattern.
@m4dc4p
Copy link
Author

m4dc4p commented Jan 19, 2018

To test this fix, first download https://download.libsodium.org/libsodium/releases/libsodium-1.0.16-msvc.zip and (from that archive) copy x64\Release\v141\dynamic\libsodium.dll (or Win32, if on a 32-bit OS) to a directory on your PATH so Kalium will find the DLL.

  1. To see test failures, clone master and run tests without this fix. A bunch will fail due to the version string not being parsed correctly.
  2. Switch to this branch and run all tests again. They will all pass.

Copy link

@dmitry-timofeev dmitry-timofeev left a comment

Choose a reason for hiding this comment

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

Great, thank you!

We faced the same issue, when tried to use kalium on Windows with the latest version of the libsodium. @abstractj , would it be possible to integrate this patch? Thanks!


@Test
public void testSodiumVersion() {
Assert.assertTrue(NaCl.sodium().sodium_version_string() + " did not match expected pattern.",

Choose a reason for hiding this comment

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

Could we use a matcher to simplify code and improve error reporting:

assertThat(version, matchesPattern("^\\d+\\.\\d+\\.\\d+$"));


@Test
public void testSodiumVersion() {
Assert.assertTrue(NaCl.sodium().sodium_version_string() + " did not match expected pattern.",

Choose a reason for hiding this comment

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

I'd add static imports for such commonly used and familiar methods (like assertX, mock/verify, various matchers).

@m4dc4p
Copy link
Author

m4dc4p commented Jul 5, 2018 via email

@dmitry-timofeev
Copy link

@m4dc4p , I see, thank you very much for the recommendation and the investigation of this issue. Will check that library and see if it works for us!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants