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

[BUG] Compliance with incomplete standard library #52

Closed
jfalcou opened this issue Nov 15, 2022 · 25 comments
Closed

[BUG] Compliance with incomplete standard library #52

jfalcou opened this issue Nov 15, 2022 · 25 comments
Labels
bug Something isn't working

Comments

@jfalcou
Copy link
Owner

jfalcou commented Nov 15, 2022

Apple clang miss some implementation and probably misses others.

@jfalcou jfalcou added the bug Something isn't working label Nov 15, 2022
@jfalcou
Copy link
Owner Author

jfalcou commented Nov 15, 2022

@DoDoENT
Copy link

DoDoENT commented Nov 15, 2022

Does this setup looks similar to yours ?
https://github.com/actions/runner-images/blob/main/images/macos/macos-11-Readme.md

Almost - it does have Android NDK r25b, but Xcode version is a bit old (13.2.1 - there were two updates with compiler upgrades: 13.3.x and 14.x - 13.3 got some concepts, but still not everything that upstream libc++ has and 14.x has most of the concepts library in its libc++). NDK r25, however, still uses libc++ from LLVM 11 (or maybe 12, but definitely not something more recent).

In general, if you make it work with this runner, it will most probably work with all newer compilers as well...

This setup is actually more similar to what we have (I work with @psiha)...

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 16, 2022

OK, I'll setup that and see how many things break :D

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 16, 2022

@DoDoENT @psiha Would you mind checking #56 and see if I need to put special options or if I need to trigger more build on the Mac OS X machines ?

@psiha
Copy link
Contributor

psiha commented Nov 16, 2022

not sure what you meant by "trigger more build"? :)

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 16, 2022

Should I try compiling with somethign else than clang mac and g++ mac ?
Atm, it looks like no crap happens with Apple Clang in the test I enabled.

@DoDoENT
Copy link

DoDoENT commented Nov 16, 2022

Not that it's important, but both GCC mac and clang mac use xcode's apple-clang in your tests.

It's not important because we are mostly interested in AppleClang 14.0.0, which is used in your tests (even when you supposedly don't want it to 😛 )

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 16, 2022

Ok wait, did I screw up then or is it the normal behavior ?

@DoDoENT
Copy link

DoDoENT commented Nov 16, 2022

I don't know - I never used Github actions (we have internal Jenkins for driving our builds in the company). I just noticed that both gcc-mac and clang-mac in your CI use the same compiler.

However, from what I see (based on CMake output), AppleClang 14.0.0 is used, which is OK - it's the one shipped with Xcode and the one that needs to be used for iOS builds, although here it's used only for mac builds. iOS builds are very similar, but some features may be missing as they require the latest iOS version or are mac-only. Those are primarily OS-specific features, such as TLS support, STL filesystem, etc. I guess you don't use any of those in your code.

@psiha
Copy link
Contributor

psiha commented Nov 17, 2022

@DoDoENT I guess then🍎 finally added the minimum necessary concepts with v14/latest Xcode? The only thing we miss from Xcode14 then is std::bit_cast

@jfalcou so I guess we're out of luck - Android is the bad boy now with missing concepts and <bit> contents.. and Github, I suppose, certainly does not offer even cross-compiling for Android..

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 17, 2022

@psiha I see a bunch of Android related tools and SDK listed in the mac-12 image. My main issue is that I have 0 experience using them. So in practice we can probably add them as CI target but some pointers to how they have ot be used would be nice.

Ripped from the macos-12 image doc:

Android

Package Name Version
Android Command Line Tools 7.0
Android Emulator 31.3.13
Android SDK Build-tools 33.0.0
32.0.0
31.0.0
30.0.0 30.0.1 30.0.2 30.0.3
29.0.0 29.0.1 29.0.2 29.0.3
28.0.0 28.0.1 28.0.2 28.0.3
27.0.0 27.0.1 27.0.2 27.0.3
Android SDK Platforms android-33 (rev 2)
android-32 (rev 1)
android-31 (rev 1)
android-30 (rev 3)
android-29 (rev 5)
android-28 (rev 6)
android-27 (rev 3)
Android SDK Platform-Tools 33.0.3
Android Support Repository 47.0.0
CMake 3.18.1
3.22.1
Google Play services 49
Google Repository 58
NDK 23.2.8568313
24.0.8215888
25.1.8937393 (default)
SDK Patch Applier v4 1

Environment variables

Name Value
ANDROID_HOME /Users/runner/Library/Android/sdk
ANDROID_NDK /Users/runner/Library/Android/sdk/ndk/25.1.8937393
ANDROID_NDK_HOME /Users/runner/Library/Android/sdk/ndk/25.1.8937393
ANDROID_NDK_LATEST_HOME /Users/runner/Library/Android/sdk/ndk/25.1.8937393
ANDROID_NDK_ROOT /Users/runner/Library/Android/sdk/ndk/25.1.8937393
ANDROID_SDK_ROOT /Users/runner/Library/Android/sdk

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 17, 2022

I am gonna merge the MAC OS runner now and keep this open till I find a proper way to test for android.

@DoDoENT
Copy link

DoDoENT commented Nov 17, 2022

My main issue is that I have 0 experience using them.

In general, all you need to do is build the code using NDK toolchain. There is no need to run tests, as that requires real Android device or emulator.

To build CMake-based project using NDK, all you need to do is provide NDK's toolchain to the CMake invocation. The toolchain is located in ANDROID_NDK_ROOT/build/cmake/android.toolchain.cmake.

Then, the minimal incantation should be something like cmake -GNinja -DCMAKE_TOOLCHAIN_FILE=$ANDROID_NDK_ROOT/build/cmake/android.toolchain.cmake -DANDROID_ABI=arm64-v8a /path/to/source.

To compile for ARMv7, use -DANDROID_ABI=armeabi-v7a. I think that the default should work fine. In our build system, we additionally also define the following CMake variables: -DANDROID_PLATFORM=android-19 -DANDROID_STL=c++_static -DANDROID_TOOLCHAIN=clang and -DANDROID_ARM_NEON=ON for ARMv7, but I guess those should also be defaults in NDK r25b.

Then simply use ninja for building as you normally would for desktop platforms (or remove -GNinja from CMake invocation to generate Unix Makefiles instead and simply use make (make is a lot slower than ninja))

@DoDoENT
Copy link

DoDoENT commented Nov 17, 2022

Similarly, to test building for iOS, you can do this:

cmake -GXcode -DCMAKE_SYSTEM_NAME=iOS /path/to/source

The only requirement here is to have CMake 3.24 or newer to have CMake properly generate Xcode 14-compatible project (earlier versions fail to do so).

Then simply build with cmake --build . --config Release as this is much easier than figuring out correct invocation parameters of the xcodebuild tool.

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 17, 2022

Thanks a lot, I will give that try.

@psiha
Copy link
Contributor

psiha commented Nov 19, 2022

We're thinking of extracting and publishing these 'poor-man's STL complements' (for targets which force you to use 'what they give you', such as iOS and Android) - and we're open to some herpderping ;D from your side on how to name it:

  • one way would be for the 'library' to have no prefix directory and use exactly the same names as the standard ones, relying on #include_next chemistry to delegate to the system/default header (e.g. you'd #include <bit> and it would include the 'complementary'/'patch' header which would internally include the system one and fill in the missing pieces) - we're leaning against this as it seems flakey (that in some circumstances it may not work as expected - and only/directly the broxen system header will get included)
  • the other way (other than using totally nonstandard names) would be to just use a prefix include directory - say #include <std_fix/bit> - but then you have to either always use these 'patch' headers or use both while ifdefing for the problematic targets...
    ?
    (or should I have put this under discussions too?:)

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 19, 2022

  • include_next is turbo flakey. We trie dit in EVE to simplify the "fetch proper arch include" and it was meh
  • std_fix/* sounds the simplest safe option IMHO. Just put the #ifdef inside it and forget it. We had had a eve/detail/concepts for a while that just did that.

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 19, 2022

Good news: @DoDoENT tips are working perfectly.
Bad news: the std is indeed fu. As it breaks inside TTS our test system and Kumi, I am going to add Android testing + Mac testing to those and fix those issue there then migrate the fix.

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 21, 2022

@DoDoENT github mac os x image are said to have "Android Emulator v31.3.13". Could it be used easily ?

Everythign has been ported, now I have to fight the missing libcpp partds.

@DoDoENT
Copy link

DoDoENT commented Nov 22, 2022

We generally have a bad experience with Android emulators, especially on Intel, as NDK compilers for Intel are even more buggy than those targeting ARM (for some weird reason they pack different compilers for different ABIs, instead of just using one that supports multiple targets as Apple does).

However, the emulator may work for simple tests like in this repository, but your mileage may vary.

@psiha
Copy link
Contributor

psiha commented Nov 22, 2022

These simple patches are enough for us at the moment https://github.com/microblink/std_fix ... If you like we can use a public repo like this to jointly track these defficiencies (or you can do your own of course).

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 22, 2022

I'll gladly contribute to this and use it there. I have a bunch of other fixes scattered in other libraries that I can put there

@jfalcou
Copy link
Owner Author

jfalcou commented Nov 22, 2022

Fixed in 44955d0

@jfalcou jfalcou closed this as completed Nov 22, 2022
@psiha
Copy link
Contributor

psiha commented Nov 22, 2022

I'll gladly contribute to this and use it there. I have a bunch of other fixes scattered in other libraries that I can put there

Ok, added you as a maintainer...

Also added some simple compile-time tests - we have a rather complex CI system (that is also able to run tests on mobile targets) which cannot be pluged into/with the Github CI - so we'll leave it to you to setup the Github CI as/when you put your stuff in ;D
Feel free to change the licensing.

@DoDoENT just had a need for constexpr std::uninitialized_* algorithms (which Apple and Android STLs do not provide) - and also later saw your related talk :D so he'll be adding those to at some point..

@psiha
Copy link
Contributor

psiha commented Nov 30, 2022

android/ndk#1808 :D upvote :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants