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

update for Android NDK r23 (LTS) #564

Merged
merged 2 commits into from
Oct 5, 2021

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Aug 31, 2021

In short:

Quickly tested building with this update together with PR #563, seems to work with sqlcipher-android-tests

In case I overlooked something and we should not get rid of -fuse-ld=bfd, I will understand and would appreciate a brief explanation.


P.S.:

  • encountered many, many ugly warnings when building with NDK r23, see below
  • I got this working with OpenSSL 3.0.0-beta2 with both ANDROID_NDK_HOME & ANDROID_NDK_ROOT defined ... got some deprecated OpenSSL API notes through.

@brodycj
Copy link
Contributor Author

brodycj commented Aug 31, 2021

I did notice many, many ugly warnings when building with NDK r23 like this:

armv7a-linux-androideabi16-clang  -I. -Iinclude -fPIC -pthread -Wa,--noexecstack -Qunused-arguments -Wall -O3 -march=armv7-a -fPIC -fstack-protector-all -DOPENSSL_USE_NODELETE -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DPOLY1305_ASM -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib/engines-1.1\"" -DNDEBUG -D__ANDROID_API__=16 -D_FILE_OFFSET_BITS=32 -MMD -MF apps/apps.d.tmp -MT apps/apps.o -c -o apps/apps.o apps/apps.c
In file included from <built-in>:372:
<command line>:18:9: warning: '__ANDROID_API__' macro redefined [-Wmacro-redefined]
#define __ANDROID_API__ 16
        ^
<built-in>:365:9: note: previous definition is here
#define __ANDROID_API__ __ANDROID_MIN_SDK_VERSION__
        ^
1 warning generated.

I did also encounter this when I tried building from the master branch of OpenSSL from GitHub. I suspect that this is something that should be fixed on the OpenSSL side and hope to investigate this further at some point.

@developernotes
Copy link
Member

Hi @brodybits

We would like to move to the latest LTS version of the Android NDK with the next public release of SQLCipher/SQLCipher for Android. That said, how do you feel about adjusting the README to specify the exact LTS version (i.e., 23.0.7599858) instead of just 23? Thanks!

@brodycj
Copy link
Contributor Author

brodycj commented Aug 31, 2021

Hmm ... my understanding of LTS is that they would make updates as needed for security and other critical issues for a longer support period. My thinking is that it would be nice if we did not have to keep updating the README every time they make a patch to NDK r23. To me "r23 (LTS)" means that we (you) are using the latest patch of r23 every time you make a build, and recommend the same for someone else building from source.

Keeping something like "r23 (LTS)" should also be more consistent with using the latest patch of OpenSSL 1.1.1 as well.

But please let me know if you are still thinking differently, thanks.

A couple side questions:

As I said before, OpenSSL build displays many (many) ugly warnings with NDK r23 (noticed this when I tried OpenSSL 3.0.0-beta2 as well), don't know if we should try to get this fixed on OpenSSL someday or if we may be able to fix this in our build script.

And sorry if this is way too far off-topic: I am assuming that the next public release would include recent SQLite 3.36.0, OpenSSL 1.1.1l, and recent bug fixes, and is pending your company's update/build/test process ... right?

(I think many people will be happy to see the new update :)

@developernotes
Copy link
Member

Hi @brodybits

I referenced that specific version because the Android NDK download page does in conjunction with labeling it as LTS.

Screen Shot 2021-08-31 at 13 24 31

I think it would be better to be specific about the version of the NDK that is tested and supported with the build going forward, not just the latest LTS. That way, if the NDK team issues a new 23.*version, or potentially a new LTS, the version of the NDK is explicit in terms of what we know will work with SQLCipher for Android. Over the years we've run into several problems when attempting to use newer versions of the NDK, only for those issues be fixed in future NDK releases, and so I believe it would be beneficial to vet any new NDK versions before exclaiming support.

@developernotes
Copy link
Member

Hi @brodybits

Yes, the next SQLCipher release will likely include the SQLite 3.36.0 upstream (assuming no additional SQLite releases occur), as well as the latest OpenSSL. In terms of support OpenSSL 3.0, we will certainly review that, however, we have no plans to ship a beta version of OpenSSL with a production version of SQLCipher.

@brodycj brodycj marked this pull request as draft August 31, 2021 18:36
@brodycj brodycj marked this pull request as ready for review August 31, 2021 18:51
@brodycj
Copy link
Contributor Author

brodycj commented Aug 31, 2021

I made the update, added a paragraph split for readability, and rebased.

Thanks for the info. +1 for not using OpenSSL beta in production

brodycj and others added 2 commits August 31, 2021 16:11
* use specific Android LTS version 23.0.7599858
* add a paragraph split

Co-authored-by: Christopher J. Brody <chris.brody+brodybits@gmail.com>
Co-authored-by: Nick Parker <nparker@zetetic.net>
@brodycj brodycj marked this pull request as ready for review August 31, 2021 21:11
@developernotes developernotes merged commit 17fa07d into sqlcipher:master Oct 5, 2021
@developernotes
Copy link
Member

Hi @brodybits

Just merged into master, thanks again!

@brodycj brodycj deleted the android-ndk-r23-update branch January 9, 2023 23:07
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