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

android-ndk: do not use wrapper scripts for clang #17636

Conversation

vbieleny
Copy link
Contributor

Specify library name and version: android-ndk/all

This changes are meant to fix the issue of .cmd wrapper scripts for clang, which is described here android/ndk#1856. I stumbled across this issue when I was trying to cross build (Windows to Android) libsodium package with android-ndk as a build requirement.


@CLAassistant
Copy link

CLAassistant commented May 20, 2023

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ericLemanissierBot
Copy link

ericLemanissierBot commented Jun 30, 2023

I detected other pull requests that are modifying android-ndk/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 3 (20de918b77bd19b1f4a63e4ebd68c7e650781fa9):

  • android-ndk/r25c@:
    All packages built successfully! (All logs)

  • android-ndk/r21d@:
    All packages built successfully! (All logs)

  • android-ndk/r25b@:
    All packages built successfully! (All logs)

  • android-ndk/r25@:
    All packages built successfully! (All logs)

  • android-ndk/r24@:
    All packages built successfully! (All logs)

  • android-ndk/r23@:
    All packages built successfully! (All logs)

  • android-ndk/r23b@:
    All packages built successfully! (All logs)

  • android-ndk/r23c@:
    All packages built successfully! (All logs)

  • android-ndk/r22b@:
    All packages built successfully! (All logs)

  • android-ndk/r21e@:
    All packages built successfully! (All logs)

  • android-ndk/r22@:
    All packages built successfully! (All logs)

  • android-ndk/r19c@:
    All packages built successfully! (All logs)

  • android-ndk/r20b@:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds may be required once they are on the v2 ready list

All green in build 3 (20de918b77bd19b1f4a63e4ebd68c7e650781fa9):

  • android-ndk/r25c@:
    All packages built successfully! (All logs)

  • android-ndk/r25b@:
    All packages built successfully! (All logs)

  • android-ndk/r22@:
    All packages built successfully! (All logs)

  • android-ndk/r25@:
    All packages built successfully! (All logs)

  • android-ndk/r22b@:
    All packages built successfully! (All logs)

  • android-ndk/r23c@:
    All packages built successfully! (All logs)

  • android-ndk/r23@:
    All packages built successfully! (All logs)

  • android-ndk/r24@:
    All packages built successfully! (All logs)

  • android-ndk/r23b@:
    All packages built successfully! (All logs)

  • android-ndk/r21e@:
    All packages built successfully! (All logs)

  • android-ndk/r21d@:
    All packages built successfully! (All logs)

  • android-ndk/r20b@:
    All packages built successfully! (All logs)

  • android-ndk/r19c@:
    All packages built successfully! (All logs)

danimtb
danimtb previously approved these changes Aug 17, 2023
@jcar87 jcar87 dismissed danimtb’s stale review August 17, 2023 08:25

pending review questions

Copy link
Member

@danimtb danimtb left a comment

Choose a reason for hiding this comment

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

After a closer look, I have some questions regarding the changes proposed. Thank you

Comment on lines +308 to +311
# For x86 targets prior to Android Nougat (API 24), -mstackrealign is needed to properly align stacks for global constructors. See https://github.com/android/ndk/issues/635.
# https://android.googlesource.com/platform/ndk/+/refs/heads/ndk-release-r21/docs/BuildSystemMaintainers.md#additional-required-arguments
if self.settings_target.arch == "x86" and int(str(self.settings_target.os.api_level)) < 24:
compiler_flags.append("-mstackrealign")
Copy link
Member

Choose a reason for hiding this comment

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

I think this flag is not needed for newer versions of the ndk, as since 2018 this is already fixed: android/ndk#635 (comment)

Which ndk version are you using?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using NDK version 23.1.7779620 (r23b). The change you linked appears to be a fix for the wrapper scripts. Since this PR aims to stop using the wrapper scripts, the flags need to be passed manually.

Comment on lines +305 to +306
# https://github.com/android/ndk/issues/1856
compiler_flags = [f"--target={self._clang_target}"]
Copy link
Member

Choose a reason for hiding this comment

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

Having a closer look at the issue it is not clear this solves the issue. As it is still in discussion, I would like to know if you have checked this isolated fix by yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue I encountered closely resembled one I experienced when building my project with the libsodium package as a dependency. I traced the problem to the wrapper scripts, as described here, which resolved the issue for me.

I then attempted to modify the android-ndk recipe so I wouldn't have to manually adjust the NDK scripts on my machine. I tested the build both without my changes in the recipe (where it failed) and with them (where it succeeded). I've been using the android-ndk package with these modifications for several months now and haven't faced any issues. However, I should note that I only use it for cross-compiling from Windows to Android.

If necessary, I can provide a minimal reproducible example to demonstrate the issue.

@stale
Copy link

stale bot commented Oct 15, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 15, 2023
@ericLemanissierBot ericLemanissierBot mentioned this pull request Feb 20, 2024
3 tasks
@ericLemanissierBot ericLemanissierBot mentioned this pull request Feb 29, 2024
3 tasks
Copy link
Contributor

github-actions bot commented Mar 6, 2024

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@github-actions github-actions bot closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants