-
Notifications
You must be signed in to change notification settings - Fork 264
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
ev-compile-cpp-source regression in r14 beta 2 #302
Comments
epoll_create1 and cfsetspeed were both introduced in API level 21, so it's expected that they wouldn't be visible at 14... |
but boost can use |
We didn't used to have The |
@DanAlbert I agree that in both cases boost did not do their best to protect against hostile build environment. They don't have I think that both cases can be safely resolved without changing boost code, and being a regression in the r14 beta, don't justify filing bug reports at https://svn.boost.org/trac/boost/newticket. Regarding EPOLL_CLOEXEC: I believe that As for cfsetspeed(), I believe that NDK r13 compiled our code 'by lucky chance'. It so happens that with Now the things get weird. In unified headers, On the other hand, on Android platform 21 and higher, cfsetspeed() is defined, and boost will be OK with it, regardless whether But for platform less than 21, while cfsetspeed() is missing, it is enough to |
Hiding Agreed that the
I'm confused about the rest of this though. You're saying that with r13 you didn't have |
(we might want to remove the duplication and have the specific case of missing system call stubs like as for changing |
I think I typo'd something in my comment, but now I'm having trouble untangling things. Based on the behavior (boost compiles fine),
Yeah, this is doable. I think boost needs to make some fixes anyway because of the epoll issue though, so I'd rather avoid respinning. I'll go file bugs with boost and see how accommodating they are. |
https://android-review.googlesource.com/342780 removes the EPOLL_CLOEXEC duplication. |
Hiding EPOLL_CLOEXEC in unified headers for API < 21 cannot, IMHO, break someone's code even if they use dlsym, because this simply reflects the current situation in platform headers. |
They could |
And, actually, that'd probably be the simplest fix for boost. |
The story of _BSD_SOURCE defined or not defined in our file is purely academic. It just happens so that the chain of includes with r14 w/o unified headers includes <features.h> and this causes the wrong side of |
Okay, so yes, I did misunderstand you earlier (you actually said it quite clearly in the OP, but apparently I can't read). It's the I suppose we can fix that the same way we plan to for the unified headers: we'll just add the inline definition to the deprecated headers. With that fixed you should be able to keep using the old headers if you like, or with a small fixup to boost to use |
Well I have forged a workaround for cfsetspeed() as soon as I understood where _BSD_SOURCE came from. I believe that we will not switch to unified headers with r14, but the command-line workaround for epoll_create1() is easy, as well:
By the book, this would be even more precise:
|
I've uploaded a fix for the old headers so you should be able to remove your workaround when r14 ships: https://android-review.googlesource.com/c/343306/
Any other issues we can help with? We're going to be turning these on by default in r15 unless things go horribly wrong (not sure if you've seen our roadmap before). Thanks a ton for trying these out early, by the way. It helps a lot. |
The |
Test: make checkbuild # with my versioner-in-build patches Bug: android/ndk#302 Change-Id: Ib00b5dadf23592d101486b4f2188285ec03c9e2a
I appreciate your quick response times. I don't know of specific issues with unified headers, but I prefer to make small steps, and testing the result on the real users every time. There exist too many weird devices on this planet, and some require very special shims. I guess we will be ready to switch when we upgrade to r15. |
This fixes some issues with using boost's asio (which wrongly assumes _BSD_SOURCE implies that these things are available) following our change from -isystem to --sysroot. Test: Manual, check that we can build when using cfsetspeed on ICS. Bug: android/ndk#302 Change-Id: Iab50221e4864f9a09a8fb00691252170eb6e8d09
This fixes some issues with using boost's asio (which wrongly assumes _BSD_SOURCE implies that these things are available) following our change from -isystem to --sysroot. Test: Manual, check that we can build when using cfsetspeed on ICS. Bug: android/ndk#302 Change-Id: Iab50221e4864f9a09a8fb00691252170eb6e8d09 (cherry picked from commit 3b59037)
Bug: android/ndk#302 Test: builds Change-Id: Ia3074326a128c38f2488e342c028cc030801cfd9
FYI, build 3770861 is the one I'm expecting to push to stable later this week. If you're feeling adventurous you can download it now by following https://android.googlesource.com/platform/ndk/+/master/docs/ContinuousBuilds.md |
@DanAlbert I can wait for the official release; I am much more concerned with the patch for Clang x86 stack protection, which is not expected to be available this week. |
Test: None, markdown only Bug: android/ndk#302 Change-Id: I0cf5dcea4abe5f719bf77050c3bfcbcae05c8e9a
@DanAlbert I tried the continuous build #3996500 from https://android.googlesource.com/platform/ndk/+/master/docs/ContinuousBuilds.md, and it didn't solve the problem. Still getting compilation errors:
|
Something has gone wrong with your build.
This line doesn't appear anywhere in that build, so one of the projects you're including there is probably the one including |
@DanAlbert Doing a machine wide search located this for me: /android-ndk-r16-canary/platforms/android-14/arch-x86/usr/include/linux - where you can see |
(looks like current boost says #if defined(__ANDROID__)
# ifndef PAGE_SIZE
# define PAGE_SIZE 4096
# endif
#endif instead: http://www.boost.org/doc/libs/1_64_0/boost/thread/pthread/thread_data.hpp) |
I see plenty of includes of $ grep -r 39983 .android-ndk-r16-canary/ Turns up nothing. The specific header you've pointed at shouldn't be used in your build anyway. Those are the deprecated headers, not the ones that should have this fixed. |
the line with the bug URL was a boost file. but like i said, the current version looks different. so it looks like an out of date boost that won't work with unified headers. |
That URL is being reported by Anyhow, the solution is to downgrade to Android Studio 2.3.2 and ndk-r13b. |
Some third-party code uses the existence of EPOLL_CLOEXEC to detect the availability of epoll_create1. This is not correct, since having up-to-date UAPI headers says nothing about the C library, but for the time being we don't want to harm adoption to the unified headers. We'll undef EPOLL_CLOEXEC if we don't have epoll_create1 for the time being, and maybe revisit this later. Test: make checkbuild Bug: android/ndk#302 Bug: android/ndk#394 Change-Id: I8ee9ce62768fb174070ec51d114f477389befc4a
Hello, https://android-review.googlesource.com/c/platform/bionic/+/401372 breaks building CPython with NDK r15+ and android-19 (https://bugs.python.org/issue28914). Is there a chance to revert the aforementioned change in r17? I guess after r15 and r16 cycles, most third-party codes have been fixed. |
because of the workaround, no third-party code will have changed: the workaround ensures that they continue to build because they won't see the constants if the function isn't available. |
Thanks I guess you're right. I'll go back to CPython for a fix. |
python/cpython@b8d9032 (using configure to detect epoll_create1) looks like the right fix. |
We did actually change the unified headers to not expose constants before the corresponding functions are available (at least in the epoll/inotify cases that caused trouble, plus sync_file_range which we spotted in passing). Bug: android/ndk#302 Bug: android/ndk#394 Test: N/A Change-Id: Ia46b56862475f68a8ae3fa3677532c828eec390c
This is a partial https://android-review.googlesource.com/c/343952/. We shipped this in r14, but r15 beta 1 is shipping the O preview headers, which is older than this change. Test: ndk/checkbuild.py && ndk/validate.py Bug: android/ndk#302 Change-Id: Icbc8693bcdead9195bf8a74978a9a684048ef5a9 (cherry picked from commit 8af1f77ac680bba3535e83d2b06208e82230a81c)
Bug: android/ndk#302 Test: builds Change-Id: Ia3074326a128c38f2488e342c028cc030801cfd9
Bug: android/ndk#302 Test: builds Change-Id: Ia3074326a128c38f2488e342c028cc030801cfd9
Bug: android/ndk#302 (comment) Test: builds Change-Id: Ia3074326a128c38f2488e342c028cc030801cfd9
Bug: android/ndk#302 Test: builds Change-Id: Ia3074326a128c38f2488e342c028cc030801cfd9
Description
Same C++ code (boost 1.63's asio/detail/impl/epoll_reactor.ipp) compiles in r13, but fails in r14β2. I could trace the change back to file
definitions.mk
, scriptsev-compile-cpp-source
and her kin. In r13, the_FLAGS
end withWhile in r14, this has changed to
Now if we call
ndk-build
withAPP_UNIFIED_HEADERS=true
, the compile command ends withand the result is:
If I use
ndk-build
withAPP_UNIFIED_HEADERS=false
, the compile command line does not contain-isystem
at all, with equally unpleasant result:Environment Details
The text was updated successfully, but these errors were encountered: