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 NOTES.ANDROID for newer NDK versions + small fixes. #10478

Closed
wants to merge 2 commits into from

Conversation

@Kagetsuki
Copy link
Contributor

Kagetsuki commented Nov 20, 2019

Fixes #8941

This has only been tested by myself - it would be good if someone can check the accuracy of new information/instructions before merging.

The newer NDK's don't include GCC and have different structures and paths, especially if you are using the new side-by-side NDK. I've retained the information for older NDK's as it's still relevant for people who are not updating their toolchains (EG maintaining somewhat legacy apps).

Checklist
  • documentation is added or updated
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Nov 20, 2019

Isn't this update supposed to include information for NDK 20+? It seems to be missing...

@Kagetsuki

This comment has been minimized.

Copy link
Contributor Author

Kagetsuki commented Nov 20, 2019

Isn't this update supposed to include information for NDK 20+? It seems to be missing...

There are several references, but you made me realize I forgot to include part of the path needed for the clang toolchain. I've updated the document.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Nov 20, 2019

Cool. Do you need to say anything special to get clang? I usually have to say CC=clang on my linux builds...

@Kagetsuki

This comment has been minimized.

Copy link
Contributor Author

Kagetsuki commented Nov 21, 2019

Cool. Do you need to say anything special to get clang? I usually have to say CC=clang on my linux builds...

No extra arguments needed! clang detection was added as a fallback for when GCC is not found at some point. The part of the path that I had forgotten to add that your comment reminded me of was actually the path to the Android NDK llvm/clang tools, which for some reason are in a different path than all the other tools... Otherwise there really wasn't much of a big change for r20+ other than a few caveats (like paths being slightly different for side-by-side NDK, which is now the "recommended" way to obtain it).

I'm currently trying to get Gradle to build out all platforms through the side-by-side NDK and their new external build tools. I suppose if I get that working I should add that to the document as well and put up another PR - but it's not exactly vital information just "convenient" to some.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Nov 21, 2019

Thanks for the explanation

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Nov 21, 2019

Travis failure is not relevant to this change but is interesting (@paulidale):

    # ERROR: (int) 'errors < max - tail' failed @ test/property_test.c:379
    # [9990] compared to [9990]
    not ok 8 - test_query_cache_stochastic
openssl-machine pushed a commit that referenced this pull request Dec 3, 2019
Fixes #8941

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10478)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2019
Fixes #8941

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10478)

(cherry picked from commit d3a27c5)
@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Dec 3, 2019

Another that dropped off the radar. Merged to master and 1.1.1.
Travis failure is not related.

@petrovr

This comment has been minimized.

Copy link

petrovr commented Dec 3, 2019

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Dec 3, 2019

We can't always double check things ourselves, so we have to trust our community.
If you find an error, feel free to submit a correcting PR

@petrovr

This comment has been minimized.

Copy link

petrovr commented Dec 3, 2019

We can't always double check things ourselves, so we have to trust our community.
If you find an error, feel free to submit a correcting PR

Vote of openssl team member is important. Vote of other does not make sense if I remember rules.
As result (even for changes from team members) team member must be more precise - double check is required in all cases.

I don't have enough spare time to propose changes.
In addition I have enough experience to use work-around broken build system. But in this case build system is fine and so not precise documentation is out of my scope.

DDvO added a commit to mpeylo/cmpossl that referenced this pull request Dec 29, 2019
Fixes openssl#8941

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#10478)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.