bump(main/fish): 4.0.0

[TomJo2000]

This PR is intended for build testing.

DO NOT MERGE.

Fish 4.0 transitions the project to Rust.
Meaning we gotta write new patches.

CC: @AminurAlam

[Biswa96]

This probably needs some work for Android. For example, the first error was fixed in libc 0.2.165 version https://github.com/rust-lang/libc/releases/tag/0.2.165

[TomJo2000]

We're probably not going to wanna package the beta release anyway, this is just intended for testing.

[thunder-coding]

Assigning myself as I use the fish shell, so will try to test this out

[TomJo2000]

Push access for maintainers is enabled on this PR.
So feel free to push to this branch as necessary, or open up a new branch/PR.

[thunder-coding]

Rust's libc has no support for posix_spawn primitives. I'll create an issue upstream and try to create a PR as well with the fix.

The integer conversion errors also seem to be Rust libc bugs:

error[E0308]: mismatched types

   --> src/wutil/dir_iter.rs:115:50
    |
115 |             self.typ.set(stat_mode_to_entry_type(s.st_mode));
    |                          ----------------------- ^^^^^^^^^ expected `u16`, found `u32`
    |                          |
    |                          arguments to this function are incorrect

ino_t is defined as c_ulong whereas d_eno is of type u64. The correct behaviour is that ino_t should also be defined as ino_t like how it is done for Linux.
https://github.com/rust-lang/libc/blob/1867bf30eb9d22d3f8e86c2a8c25cf92fe5580da/src/unix/linux_like/android/mod.rs#L24
https://github.com/rust-lang/libc/blob/1867bf30eb9d22d3f8e86c2a8c25cf92fe5580da/src/unix/linux_like/android/mod.rs#L531

error[E0308]: mismatched types
   --> src/wutil/dir_iter.rs:115:50
    |
115 |             self.typ.set(stat_mode_to_entry_type(s.st_mode));
    |                          ----------------------- ^^^^^^^^^ expected `u16`, found `u32`
    |                          |
    |                          arguments to this function are incorrect

mod_t definition
https://github.com/rust-lang/libc/blob/1867bf30eb9d22d3f8e86c2a8c25cf92fe5580da/src/unix/linux_like/android/b32/mod.rs#L8
https://github.com/rust-lang/libc/blob/1867bf30eb9d22d3f8e86c2a8c25cf92fe5580da/src/unix/linux_like/android/b64/mod.rs#L8

st_mode definition:
https://github.com/search?q=repo%3Arust-lang%2Flibc+path%3A%2F%5Esrc%5C%2Funix%5C%2Flinux_like%5C%2F%2F+path%3A%2F%5Esrc%5C%2Funix%5C%2Flinux_like%5C%2Fandroid%5C%2F%2F+st_mode&type=code

mod_t should instead be c_uint and st_mode should have it's type as mod_t

[TomJo2000]

Looks like fish 4.0.0 is officially released.

• Auto update failing for fish #23531

[TomJo2000]

Looks like the 32 Bit builds are failing now.

[TomJo2000]

It doesn't find the rust compiler anymore and I have no idea why.
https://github.com/fish-shell/fish-shell/blob/eb336889b7bcb88eb0e1f3dd678ae52275280186/cmake/FindRust.cmake#L209-L259
Setting Rust_COMPILER does nothing since we are using rustup.
I almost wanna just rip this out and sub in command -v rustc.
Never seen it be this stubborn before.

I added some patch cleanup between builds so you can do repeat builds.
Still haven't deciphered CMake's hieroglyphics though.

Applying patch: Cargo.toml.patch
Downloading https://github.com/Kitware/CMake/releases/download/v3.31.4/cmake-3.31.4-linux-x86_64.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 52.4M  100 52.4M    0     0  5693k      0  0:00:09  0:00:09 --:--:-- 5451k
Downloading https://github.com/ninja-build/ninja/releases/download/v1.12.1/ninja-linux.zip
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  120k  100  120k    0     0   166k      0 --:--:-- --:--:-- --:--:--  166k
Archive:  /home/builder/.termux-build/fish/tmp/ninja-1.12.1.zip
  inflating: /home/builder/.termux-build/_cache/ninja-1.12.1/ninja  
-- Android: Targeting API '24' with architecture 'arm64', ABI 'arm64-v8a', and processor 'aarch64'
-- The C compiler identification is Clang 18.0.3
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /home/builder/.termux-build/_cache/android-r27c-api-24-v1/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Trying to use PCRE2 from the system
CMake Warning at cmake/FindRust.cmake:31 (message):
  `rustc` not found in PATH or `/home/builder/.cargo/bin`.

  Hint: Check if `rustc` is in PATH or manually specify the location by
  setting `Rust_COMPILER` to the path to `rustc`.
Call Stack (most recent call first):
  cmake/FindRust.cmake:246 (_findrust_failed)
  cmake/Rust.cmake:5 (include)
  CMakeLists.txt:28 (include)


CMake Error at cmake/FindRust.cmake:29 (message):
  `rustc` not found in PATH or `/home/builder/.cargo/bin`.

  Hint: Check if `rustc` is in PATH or manually specify the location by
  setting `Rust_COMPILER` to the path to `rustc`.
Call Stack (most recent call first):
  cmake/FindRust.cmake:246 (_findrust_failed)
  cmake/Rust.cmake:6 (find_package)
  CMakeLists.txt:28 (include)


-- Configuring incomplete, errors occurred!

[robertkirkman]

This makes the error go away if you cherry pick it and place it into the folder with the other patches.

fish-shell/fish-shell@b38551d

[robertkirkman]

error[E0412]: cannot find type off_t in this scope

that's odd, that didn't happen for me locally. i'll try again

[TomJo2000]

Why is it building /home/runner/.termux-build/libgrpc/host-build?

[robertkirkman]

that could have happened because of libgrpc being recently bumped, but that commit not having been in this branch yet
289ebee

[TomJo2000]

I didn't force push that backport up.
Possible that messed with things.
I just rebased against master.

[TomJo2000]

Anyway.
error[E0412]: cannot find type ino_t in the crate root
So ino_t isn't implemented for arm and i686 in the libc crate.
off_t as well.

[TomJo2000]

We'll need to add ino_t and off_t to this patch as well I think?
I don't have a good understanding of Rust unfortunately so I can only guess.

[TomJo2000]

@thunder-coding if you have the time it would be much appreciated if you could have another look at this.
Seeing as it's your patch and the upstream PR is also authored by you.

• android: use proper types for dirent.d_ino, dirent.d_off, stat.st_mode and stat64.st_mode rust-lang/libc#4216

[robertkirkman]

This happened because the patch failed to apply during the termux_step_pre_configure() because something inside the version of the rust libc binding crate that enters the $CARGO_HOME/registry/src/*/libc folder changed a little bit, but only for one of the files that seemed to only be compiled during the 32-bit builds.
The patch just needed to be rebased, I did that like this and it built successfully for all architectures.

diff --git a/packages/fish/libc-src-unix-linux_like-android.diff b/packages/fish/libc-src-unix-linux_like-android.diff
index ac873e2831..4c99bb7b92 100644
--- a/packages/fish/libc-src-unix-linux_like-android.diff
+++ b/packages/fish/libc-src-unix-linux_like-android.diff
@@ -139,13 +139,13 @@ Subject: [PATCH 2/2] fix more incompatible integer types
  3 files changed, 5 insertions(+), 3 deletions(-)
 
 diff --git a/src/unix/linux_like/android/b32/mod.rs b/src/unix/linux_like/android/b32/mod.rs
-index d132cbaa2590..f7534c241cb5 100644
+index 34010bb..53e2f76 100644
 --- a/src/unix/linux_like/android/b32/mod.rs
 +++ b/src/unix/linux_like/android/b32/mod.rs
-@@ -5,7 +5,9 @@ use crate::prelude::*;
+@@ -3,7 +3,9 @@ use crate::prelude::*;
+ // The following definitions are correct for arm and i686,
+ // but may be wrong for mips
  
- pub type c_long = i32;
- pub type c_ulong = u32;
 -pub type mode_t = u16;
 +pub type mode_t = c_ushort;
 +pub type ino_t = u64;

It seems like the upstream PR has been put on hold, and unfortunately I don't know a lot of Rust either only the basics, so I probably can't make a PR for this they would approve either, but I think it can safely be assumed that at some point in the future a very skilled Rust developer will wonder why their own app can't build for Android and fix it, then that fix will hopefully propagate to a future Rust libc binding crate.

[TomJo2000]

If the patch didn't apply why didn't the build stop there?

[TomJo2000]

I Frankedstein-ed @thunder-coding's patch into applying on top of rust-lang/[email protected].

It builds on 32 bit now, but I'm gonna need someone to please verify that on-device before I'm ready to declare it a victory.

[robertkirkman]

@TomJo2000 Yes this package works on device including on my 32-bit device, and I can confirm it appears to be behaving normally and seems to fully work when the appropriate build of the Termux APK is used,

and additionally, the "5u" printing problem + buggy input does go away, only when I use a build I compiled of the Termux APK containing the PR to fix that:

• Fixed: fully consume unknown CSI sequences containing non-numeric parameter byte termux-app#4417
  

[TomJo2000]

May as well mark this ready for review since it seems to build for all arches now.

[TomJo2000]

How do we actually wanna clean this up?
Should this all be squashed into 1 commit do do we want to part this out somewhat?

[robertkirkman]

In my opinion, because it is all in one package and all of the changes are required to build, and it doesn't build at individual commits in the series, it would be a good idea to squash into 1 commit, and add the description of everything into a long commit message in the commit, but we might want to hear from others since recently I was told to split a large commit into 3 separate commits in a different PR and have done so.

[TomJo2000]

I think we happen to have 1 commit that happens to have to do 3 things.
You could split it along:

• Cmake fix backport.
• Libc crate patch.
• Bump package.

But neither of them is really independent from the other two.

[TomJo2000]

To recap the dicussion from the discord.
Fish 4.0.0 seems to be unable to execute any external commands in non-interactive mode.
E.g. scripts or fish -c 'ls'.
Built in commands such as echo, pwd or exec work.
exec ls likewise works.

Reconverting to draft until we have this worked out.

[robertkirkman]

Ok, I am sorry for the delay. I was debugging Fish.
I have created a patch that is able to work around the problem.
For my devices, this patch is making these commands all work in Fish 4:

fish -c "uname -a"
fish -c "/system/bin/ls"
fish -c "ls"

Here is the patch:

--- a/build.rs
+++ b/build.rs
@@ -96,7 +96,9 @@ fn detect_cfgs(target: &mut Target) {
             Ok(target.has_symbol("localeconv_l"))
         }),
         ("FISH_USE_POSIX_SPAWN", &|target| {
-            Ok(target.has_header("spawn.h"))
+            // the command 'fish -c "uname"', and most other uses of fish's -c argument,
+            // are not working if this is enabled.
+            Ok(false)
         }),
         ("HAVE_PIPE2", &|target| {
             Ok(target.has_symbol("pipe2"))

Here is some information I have been able to obtain by debugging the behavior of this Fish package.

There is a function exec_external_command() in Fish, here.

https://github.com/fish-shell/fish-shell/blob/2930c859260433c15c1470b43987727835c1dd93/src/exec.rs#L844-L897

This is where the problem is happening, or at least a point where it's not difficult to divert the codepath away from the place where the problem is happening.

There are two possible codepaths in this function. One of them is the can_use_posix_spawn_for_job() block, which has return Err(()) and return Ok(()) in it, preventing the execution from reaching the second block in the guard clause style, and the other block is the fork_child_for_process() call, at the bottom of the function.

Because my patch uses the preprocessor-like cargo macro FISH_USE_POSIX_SPAWN to disable the block of code that is related to the problem and enable a block that is working, I believe that most likely, the references to libandroid-spawn and RUSTFLAGS+=" -C link-arg=-landroid-spawn" in the build.sh would not be necessary if you use this patch, because essentially it seems like this library's feature is what is not working currently with this version of Fish.

I do not know exactly whether the true root cause of the problem is a misbehavior on the Fish side of the posix-spawn-related code, or the libandroid-spawn side, but I did test on GNU/Linux and many Android devices, and the problem seems very consistently reproducible on Android devices, and is not reproducible on GNU/Linux.

[robertkirkman]

I believe that since there was an unexpected problem, it might be a good idea for others to test this workaround before this is merged to make sure that it is able to completely solve the problem, and also look for any other problems or possibly better solutions.

[TomJo2000]

The mention of spawn.h makes me think we think we may need to link against libandroid-spawn.
However libtree (and ldd) report that it is linking against it fine.

So this is probably for upstream to figure out.

[TomJo2000]

Ok, I am sorry for the delay. I was debugging Fish. I have created a patch that is able to work around the problem. For my devices, this patch is making these commands all work in Fish 4:

fish -c "uname -a"
fish -c "/system/bin/ls"
fish -c "ls"

Here is the patch:

--- a/build.rs
+++ b/build.rs
@@ -96,7 +96,9 @@ fn detect_cfgs(target: &mut Target) {
             Ok(target.has_symbol("localeconv_l"))
         }),
         ("FISH_USE_POSIX_SPAWN", &|target| {
-            Ok(target.has_header("spawn.h"))
+            // the command 'fish -c "uname"', and most other uses of fish's -c argument,
+            // are not working if this is enabled.
+            Ok(false)
         }),
         ("HAVE_PIPE2", &|target| {
             Ok(target.has_symbol("pipe2"))

Works on my device.
Thanks.
I think we can merge this now.