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

Stable sodium #104

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Stable sodium #104

wants to merge 8 commits into from

Conversation

lhunath
Copy link
Contributor

@lhunath lhunath commented May 19, 2018

There are a bunch of changes here:

  1. I've killed all code that installs files into the system paths. Building some code should never pollute a user's system. The fact that sudo is required is a red flag for this. A lot of this stuff was potentially overwriting existing system files (such as my local and independent install of libsodium through homebrew).
  2. LIBSODIUM_FULL_BUILD is now required but was not consistently set for all builds & platforms.
  3. build.sh's native build was conflicting with its android architecture builds. I've updated it to match gradle.build's approach, which is to build it into the host prefix. Why is this build code duplicated? We should probably kill build.sh?
  4. No more blind deleting libsodium & refetching on build. This was pointlessly excessive & expensive. Most likely, this was necessary because of build.sh's bad behavior of building the native build without a prefix (see [3]).

@lhunath
Copy link
Contributor Author

lhunath commented May 20, 2018

I suspect this build fails because the jni library isn't found by System.loadlibrary. Not exactly sure what the right place to put the library is to load it inside Travis. I would advise against some kind of global system directory.

@joshjdevl
Copy link
Owner

joshjdevl commented May 20, 2018

Thank you for the pull request. My comments are below.

I've killed all code that installs files into the system paths. Building some code should never pollute a user's system. The fact that sudo is required is a red flag for this. A lot of this stuff was potentially overwriting existing system files (such as my local and independent install of libsodium through homebrew).

This was done due to nuances of loading the library using System.loadlibrary. If the library can be loaded ok and eliminate the build error then I would be happy to not install to the system location.

LIBSODIUM_FULL_BUILD is now required but was not consistently set for all builds & platforms.

Thanks.

build.sh's native build was conflicting with its android architecture builds. I've updated it to match gradle.build's approach, which is to build it into the host prefix. Why is this build code duplicated? We should probably kill build.sh?

The various build scripts were the original build system. Gradle was a recent addition. The build scripts were kept as legacy, can be eventually deprecated and eventually removed.

No more blind deleting libsodium & refetching on build. This was pointlessly excessive & expensive. Most likely, this was necessary because of build.sh's bad behavior of building the native build without a prefix (see [3]).

There were some strange upstream changes that caused errors when updating the submodule. Though can try again the hard reset.

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