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

Fixed Android missing pthread_setcancelstate and pthread_cancel #4986

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

sheldonrobinson
Copy link

Using SomeDevHere/libbthread to add missing pthread_setcancelstate and pthread_cancel.

  1. Check for Android and include bthread.h in H5private.h
  2. Add link library libcancelthread.so

@derobins
Copy link
Member

I'd be happy to approve this, but since we have no Android testing, it'd immediately become dead code. Can you add a GitHub action to build/test on Android?

@derobins derobins changed the title Fixed android missing pthread_setcancelstate and pthread_cancel Fixed Android missing pthread_setcancelstate and pthread_cancel Oct 20, 2024
@hyoklee
Copy link
Member

hyoklee commented Oct 21, 2024

Hi, @sheldonrobinson !

Thank you so much for your patch to support bthread on Android!

However, I think your PR doesn't work if bthread is not present on system:

MakeCommand:/usr/local/bin/cmake --build . --config "${CTEST_CONFIGURATION_TYPE}"
Run command: "/usr/local/bin/cmake" "--build" "." "--config" "Release"
[  0%] Building C object src/CMakeFiles/hdf5-static.dir/H5.c.o
In file included from /home/runner/work/actions/actions/src/H5.c:21:
/home/runner/work/actions/actions/src/H5private.h:68:10: fatal error: 'bthread.h' file not found
   68 | #include <bthread.h>
      |          ^~~~~~~~~~~
1 error generated.
gmake[2]: *** [src/CMakeFiles/hdf5-static.dir/build.make:76: src/CMakeFiles/hdf5-static.dir/H5.c.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:2099: src/CMakeFiles/hdf5-static.dir/all] Error 2
gmake: *** [Makefile:166: all] Error 2
Command exited with the value: 2
MakeCommand:/usr/local/bin/cmake --build . --config "${CTEST_CONFIGURATION_TYPE}"
Error(s) when building project
   3 Compiler errors
   1 Compiler warnings

Would you please improve your PR to fix the above issue?

Copy link
Member

@hyoklee hyoklee left a comment

Choose a reason for hiding this comment

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

For CMake users, this PR requires
https://github.com/sheldonrobinson/libbthread

Once libbthread is installed properly with either autotools or cmake,
NDK can build HDF5 successfully.

@sheldonrobinson
Copy link
Author

I looked at hdf5 Github workflows and it seems we are not currently building/testing for Android. We can proceed in either of the following ways:

  1. Merge PR given that it does not break or interfere with current builds.
    1. Cons The caveat, user needs to install SomeDevHere/libbthread where I also submitted PR to fix pthread link error and don't want to fork libbthread codebase.
    2. Pros Quick and easy
  2. Add Android target build/test Github workflow.
    1. Cons Will take longer, also waiting on upstream fix to SomeDevHere/libbthread.
    2. Pros This is what needs to occur in the end.

@derobins
Copy link
Member

Without testing, this code will quickly break as we move things around. We had this problem with MinGW and Cygwin before we integrated them into our CI. People would submit patches, everything would be fine for a version or two, we'd make changes, and then everything would be broken again.

@derobins derobins added Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Component - Build CMake, Autotools Type - New Feature Add a new API call, functionality, or tool labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Build CMake, Autotools Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Type - New Feature Add a new API call, functionality, or tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants