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

Support for Android NDK r22-beta1 #13434

Conversation

@iguessthislldo
Copy link
Contributor

@iguessthislldo iguessthislldo commented Nov 18, 2020

I think builds using standalone toolchain are fine so I left them alone, but Configure will fail if using the NDK directly because the platforms and sysroot directories were removed.

If sysroot is missing, omit the --sysroot and -gcc-toolchain arguments and use the triplet form clang command.

Also since platforms was being used for the default API level, use meta/platforms.json instead if needed.

I tested this by building with r21d and r22-beta1, both using the NDK directly and using a standalone toolchain. These were just builds. I included "CLA: trivial" in my commit message, but am unsure if this is actually considered trivial or not.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 18, 2020

This looks ok to my eye, but I'd like @levitte to take a look.

I've added the "CLA: trivial" label for now, because you've flagged it as such. However I think this change goes beyond our definition of trivial and that a CLA is required:

https://www.openssl.org/policies/cla.html

@iguessthislldo
Copy link
Contributor Author

@iguessthislldo iguessthislldo commented Nov 18, 2020

I will submit one then. Do I need to force push to get rid of the "CLA: trivial" in the commit message afterwards or is it fine as is?

I think builds using standalone toolchain are fine so I left them alone,
but `Configure` will fail if using the NDK directly because the
`platforms` and `sysroot` directories were removed.

If `sysroot` is missing, omit the `--sysroot` and `-gcc-toolchain`
arguments and use the triplet form clang command.

Also since `platforms` was being used for the default API level, use
`meta/platforms.json` instead if needed.
@iguessthislldo iguessthislldo force-pushed the iguessthislldo:igtd/android-ndk-r22-beta1 branch to 417bc82 Nov 19, 2020
@iguessthislldo
Copy link
Contributor Author

@iguessthislldo iguessthislldo commented Nov 19, 2020

I went ahead and force pushed the same commit with the CLA part removed made the email address match the one I registered in the CLA.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 20, 2020

Needs second review

@levitte
Copy link
Member

@levitte levitte commented Nov 20, 2020

I can't claim understanding the whole Android structure, but this was clear enough. Good work!

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Nov 21, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

openssl-machine pushed a commit that referenced this pull request Nov 23, 2020
I think builds using standalone toolchain are fine so I left them alone,
but `Configure` will fail if using the NDK directly because the
`platforms` and `sysroot` directories were removed.

If `sysroot` is missing, omit the `--sysroot` and `-gcc-toolchain`
arguments and use the triplet form clang command.

Also since `platforms` was being used for the default API level, use
`meta/platforms.json` instead if needed.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13434)
@levitte
Copy link
Member

@levitte levitte commented Nov 23, 2020

Merged

948fd7a Support for Android NDK r22-beta1

@levitte levitte closed this Nov 23, 2020
@iguessthislldo iguessthislldo deleted the iguessthislldo:igtd/android-ndk-r22-beta1 branch Nov 27, 2020
@iguessthislldo iguessthislldo restored the iguessthislldo:igtd/android-ndk-r22-beta1 branch Nov 27, 2020
iguessthislldo added a commit to iguessthislldo/OpenDDS-Android that referenced this pull request Nov 27, 2020
Use my openssl fork until openssl/openssl#13434
makes it into a release.

Also try API Level 30

Use nproc for make -j
@wongsyrone
Copy link

@wongsyrone wongsyrone commented Dec 16, 2020

Please backport this to OpenSSL 1.1.1 branch.
#13685

iguessthislldo added a commit to iguessthislldo/openssl that referenced this pull request Dec 16, 2020
This is a backport of openssl#13434 to fix openssl#13685.

I think builds using standalone toolchain are fine so I left them alone,
but `Configure` will fail if using the NDK directly because the
`platforms` and `sysroot` directories were removed.

If `sysroot` is missing, omit the `--sysroot` and `-gcc-toolchain`
arguments and use the triplet form clang command.

Also since `platforms` was being used for the default API level, use
`meta/platforms.json` instead if needed.
iguessthislldo added a commit to iguessthislldo/openssl that referenced this pull request Dec 16, 2020
This is a backport of openssl#13434, Fixes openssl#13685.

I think builds using standalone toolchain are fine so I left them alone,
but `Configure` will fail if using the NDK directly because the
`platforms` and `sysroot` directories were removed.

If `sysroot` is missing, omit the `--sysroot` and `-gcc-toolchain`
arguments and use the triplet form clang command.

Also since `platforms` was being used for the default API level, use
`meta/platforms.json` instead if needed.
@wongsyrone
Copy link

@wongsyrone wongsyrone commented Dec 28, 2020

I fixed it on my side using #13694. Works smoothly. Good job.

iguessthislldo added a commit to iguessthislldo/OpenDDS-Android that referenced this pull request Mar 25, 2021
- Fix check for r23
- Use Latest 1.1.1
- Use alpha 3.0 since openssl/openssl#13434 was
  merged quite a awhile ago.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants