fix(main/uv): Fix #28771: Update uv#28804
Conversation
|
Does this fix this issue? Also, what about 64-bit x86 architecture, does that need a case to handle it also? |
As current pull request title denotes, yes it solves #28771. |
|
It'd be great if ya'll could wait for us to validate the correct change upstream instead of shipping incremental patches from a third-party contributor. |
When you make an upstream solution, it won't be testable yet though, right, since the current newest version of uv doesn't work on Android even with Python 3.12? (and the change might not apply without conflicts to the older version that did work) |
I agree we are blocked to uv 0.9.5, even on Python 3.12, so no matter uv upstream change we need a specific Termux patch in termux-packages repository. Waiting upstream would just avoid potentially changing our potentially different patch to upstream one to get on the same page, otherwise solving the blocking issue as quick as possible seems to be the priority in my opinion. |
|
I'm fully supportive of ya'll backporting a fix, but the last one was clearly incorrect to us. It looks like you're stuck on 0.9.15 because your previous patches didn't apply? I think that's a good reason to engage with us upstream about issues, e.g., I didn't know about #27176 until now. |
I think this is the issue in the uv repo: |
|
A user @NoNameWasDefined reported this (#28804) works for them (through Discord) |
|
@Benjamin-Loison since zanieb says that If you remove it, does the previous error return? |
|
Incorrect as-in incomplete, i.e., the architecture transformation is needed too. This patch also seems a bit weird because the |
|
astral-sh/uv#14574 looks focused us supporting builds for the target? It doesn't mention the flock patch at all. |
I think zanieb said that it was incorrect because I was just correcting |
|
I'm sorry for the confusion, I interpreted that issue as "catch-all Android support", coming with the implication that once the uv repository had an official Android build, they would test it and eventually encounter any follow-up Android-specific issues that would then become reproducible in the official build. |
It seems that I agree with you. |
This is by purpose, see #discussion_r2891055748. |
That makes sense, but I'd also happily use an android-specific flock approach so you don't need to carry a patch even if we don't have full support. Feel free to open specific issues in the future.
I'm not sure I follow, the system can report "linux" as the operating system and still report android-style architectures? |
I was unsure about this and preferred being on the safe side by converting CPU architectures for linux in case of doubt, but #discussion_r2891291290 seems to show that such mapping does not seem required for linux, I am waiting @robertkirkman confirmation before updating according my pull requests. |
ad5cc1a to
a268a0a
Compare
|
I updated accordingly both pull requests. |
c1d2d0c to
a268a0a
Compare
a268a0a to
197270b
Compare
|
Yes, now the command |
Replying from this msg sorry |
Actually what I was wondering is, now that upstream is fixing the build for Android, could you make this PR into a PR to update uv to the newest version and replace all the downstream patches with patches copied from upstream's PRs? That way, this issue could also be fixed at the same time: |
looks unclear to me, are you referring to astral-sh/uv#18333? So you don't want to wait astral-sh/uv#18323 getting merged, and want to make current PR contain astral-sh/uv#18323 to have an up-to-date Termux uv? |
If that's ok with them, yes. |
I am currently testing such an approach at Benjamin-Loison/termux-packages/issues/15. |
aa9f85d to
c59520f
Compare
|
@robertkirkman I updated current PR accordingly. |
|
I think you'll need to take astral-sh/uv#18323 (though it has not been tested and reviewed on our end yet) or uv will be unusable. |
@Benjamin-Loison I think that is a good idea, and then once it is built in a termux package, termux users can test it by downloading the PR artifact here and check the behavior. |
c59520f to
35d3c9f
Compare
I just forgot to |
|
Ok, now, could you make this cleanup change in the package? --- a/packages/uv/build.sh
+++ b/packages/uv/build.sh
@@ -13,17 +13,6 @@ TERMUX_PKG_AUTO_UPDATE=true
termux_step_pre_configure() {
termux_setup_cmake
termux_setup_rust
-
- # Dummy CMake toolchain file to workaround build error:
- # error: failed to run custom build command for `libz-ng-sys v1.1.15`
- # ...
- # CMake Error at /home/builder/.termux-build/_cache/cmake-3.28.3/share/cmake-3.28/Modules/Platform/Android-Determine.cmake:217 (message):
- # Android: Neither the NDK or a standalone toolchain was found.
- export TARGET_CMAKE_TOOLCHAIN_FILE="${TERMUX_PKG_BUILDDIR}/android.toolchain.cmake"
- touch "${TERMUX_PKG_BUILDDIR}/android.toolchain.cmake"
-
- : "${CARGO_HOME:=$HOME/.cargo}"
- export CARGO_HOME
}
termux_step_make() {when I test building this, it seems like that section is not necessary anymore, because that error does not occur anymore. Setting |
35d3c9f to
777da81
Compare
|
@robertkirkman I proceeded to your requested changes. |
|
That's great! @okhex @NoNameWasDefined @shel1kest0learn @Vinfall @DroidKali could you download this PR's GitHub Actions artifact when it finishes building and try using this build of |
I will do it, yes. I'm waiting for the build to finish. (Building it is really slow) |
|
Installing MyPy works with |
|
Everything is working properly now 👍 astral-sh/uv#9559 This issue can be closed now |
|
Ok @zanieb would you recommend we wait until your PR is finalized and merged upstream, or do you think it is fine to merge this PR now? |
|
I'm supportive of you carrying the patch without it landing upstream if it's working — it'll take us a bit longer to get an Android test pipeline figured out. |
|
Ok, I will wait a little while for anyone using this to report issues, then merge it if none are found. |
|
There are some existing issues loosely related to
however, those are edge cases specific to particular PyPi packages and should be solved separately. |
|
This should close #22422 as it fixes the |
uv_0.10.9_aarch64.deb uv pip list successful |
@robertkirkman