Fixes for MSYS2 #11907

Nahor · 2025-10-07T18:49:39Z

Fix MSYS2 compilation (missing ulimits)
Fix port selection in fish_config that did not work for MINGW python
Add a minimal test for fish_config as requested in fish_config not working in msys2 #11805
Add a github action to build MSYS2. This does not run the tests because too many are failing for me (and worse, the test just freeze/deadlock when checking universal variables)

Fixes issue #11805

TODOs:

~~Changes to fish usage are reflected in user documentation/manpages.~~ [None]
Tests have been added for regressions fixed

Note

One test was added, to ensure fish_config can be shutdown (as requested in #11805), but it does not test that a browser can access the server (because this would add a #REQUIRES for a new command, e.g. wget/curl).
I do not know if that one test is sufficient for this PR, so for now, I'm not checking that TODO

~~User-visible changes noted in CHANGELOG.rst~~ [None]

Berrysoft · 2025-10-09T06:41:29Z

src/builtins/ulimit.rs

+                pub const NPROC: libc::c_int = -1;
+            } else {
+                pub const NPROC: libc::c_int = libc::RLIMIT_NPROC as _;
+                }


Suggested change

}

}

Berrysoft · 2025-10-09T06:47:03Z

This does not run the tests because too many are failing for me...

Me, too. Some of them might be caused by MSYS2 which doesn't respect the file permissions like cygwin, but it's harder to setup a testing environment for fish on original cygwin. I don't know if cygwin/MSYS2 was tested in CI before fish 4.0, but if it wasn't, it's an acceptable status currently (maybe).

krobelus · 2025-10-09T17:47:33Z

.github/workflows/test.yml

+        shell: msys2 {0}
+    steps:
+    - uses: actions/checkout@v4
+    - uses: msys2/setup-msys2@v2


Cool so I assume this will give a Bash shell, as opposed to the default (powershell?)

krobelus · 2025-10-09T17:47:33Z

.github/workflows/main.yml

+    - uses: msys2/setup-msys2@v2
+      with:
+        update: true
+        install: >-


what does the >- syntax do? convert barewords to list?

I just copy/pasted what the tutorial for MSYS2 github action does.
And yes, it converts the multi-line word list into a single-line list (>) without a newline at the end ('-'). Not really needed here but that will be one less line change if we need to add more packages and organize them

krobelus · 2025-10-09T17:47:33Z

.github/workflows/test.yml

+        update: true
+        install: >-
+          git rust
+        msystem: MSYS


these translate to pacman -S git rust commands right?
Could we use those (so it's easier for me to copy-paste these steps onto any Windows system).
Unless there's a relevant difference of course.

In essence, it's indeed just a pacman -S.
Looking at their code, strictly speaking, it's doing a pacman --noconfirm -S --needed --overwrite '*' git rust.
It also has some caching being done so that when testing multiple environment (MSYS2, MINGW32, MINGW64, ...) it won't re-download the common packages, which we'll probably never need.

krobelus · 2025-10-09T17:47:34Z

.github/workflows/test.yml

+    - uses: actions/checkout@v4
+    - name: cargo build
+      run: |
+        cargo build


Thanks, this is already a great step in the right direction.
We could even add a smoke test like

set -x [ "$(target/debug/fish.exe -c 'echo (math 1 + 1)')" = 2 ]

(FWIW debug builds currently only work due to 012b507 (Workaround
for embed-data debug builds on Cygwin, 2025-09-16) which we should
get rid of.. there is some problem with Windows paths in rust-embed)

there is some problem with Windows paths in rust-embed

If it is built with rustc shipped with MSYS2, the paths should be in POSIX style, I assume?

We could even add a smoke test

You mean as a temporary replacement for calling cargo test, as an extra "name / run"?

krobelus · 2025-10-09T17:47:34Z

tests/checks/tmux-fish_config.fish

+#RUN: %fish %s
+
+#REQUIRES: command -v tmux
+isolated-tmux-start


One test was added, to ensure fish_config can be shutdown (as
requested in #11805), but it does not test that a browser can access
the server (because this would add a #REQUIRES for a new command,
e.g. wget/curl).
I do not know if that one test is sufficient for this PR, so for
now, I'm not checking that TODO

Right, if it's easy to add curl localhost:$port | grep some line,
that sounds like a nice addition to this test.

But I'm happy to merge without a faithful test.
Historically, this functionality sometimes broke in Cygwin and WSL,
where we don't run these tests in the CI anyway yet.

My issue was adding curl (or wget) as a new requirement for testing since I didn't see any other test doing http request.

ok. I'd normally prefer curl since it's more widely used but wget is fine too of course.
Can you maybe install wget explicitly in the main CI job (the first ubuntu one in .github/workflows/main.yml).

(Could also add it to other platforms but before we do that we should probably extract functions for installing all test dependencies, so it doesn't get any messier. For example ./.github/actions/install-sphinx-markdown-builder is one such function.)

I went with wget because it's pretty much pre-installed on any system. Like the equivalent of nano vs emacs/vi. Even my router, whose firmware has to be as small as possible, has it (OpenWRT). And it's ubiquitous enough to be a tool integrated into Busybox.

If you really want, I can add it to the Ubuntu build, it's simple enough. But it feels unnecessary. It passes all the tests, which means wget is already installed. For that matter wget is a dependency of the ubuntu-standard meta package so if it's not there, we have bigger problems.

krobelus · 2025-10-09T17:47:34Z

tests/checks/tmux-fish_config.fish

+isolated-tmux capture-pane -p | sed \
+    -e '1h;2,$H;$!d;g' \
+    -e 's|file://.*\.html|<file_url>|' \
+    -e 's|http://.*/\n|<http_url>\n|'


probably awk would be more readable than sed magic but sure,
if it works on BSD sed too.

FWIW the 80 columns are not set in stone;
it's from the isolated-tmux new-session -x 80 -y 10 line
For example the test could change it with (untested)

isolated-tmux resize-window -x 500

which is probably a better solution than working around it in sed?

Can do.
~~Alternative: redirect fish_config output to a file and use cat or tail instead of tmux capture-pane and avoid the width limit entirely.~~ Doesn't work because of buffering inside the redirect

krobelus · 2025-10-09T17:49:30Z

I don't know if cygwin/MSYS2 was tested in CI before fish 4.0

only if downstream (Cygwin packagers) tested it

krobelus · 2025-10-09T18:56:16Z

You mean as a temporary replacement for calling `cargo test`, as an extra "name / run"?

yeah. Just to test it can start and execute commands

> @@ -0,0 +1,26 @@ +#RUN: %fish %s + +#REQUIRES: command -v tmux +isolated-tmux-start My issue was adding `curl` (or `wget`) as a new requirement for testing since I didn't see any other test doing http request.

Right. Maybe use `python -c 'import requests; ...'` ? I think we can already assume that `python` exists in `PATH`.

Good idea -- write to a file in the current directory, and then use `cat log` outside tmux.

Nahor · 2025-10-10T00:35:18Z

Maybe use python -c 'import requests; ...' ?

requests is not standard Python so it won't be on all platform by default (at least it's not with MSYS2 python). urllib could be used but it feels too complicated for one-liners.
I believe wget should be safe to use.

Alternative: redirect fish_config output to a file and use cat or tail instead of tmux capture-pane and avoid the width limit entirely.
Good idea -- write to a file in the current directory,

I tried and it doesn't work. The problem is that the file is buffered so there is no data until we exit fish_config. I thought about running all the commands and then checking the output, but if we want to run some basic HTTP requests, we need the output to get the authentication string.
I tried to use tee to work around the buffering but for some reason, it's not capturing the output (terminal and file remain blank until fish_config closes at which point all the output gets printed in the terminal and none in the file)
I also thought of stdbuf but that expects an actual command, not a shell function.

krobelus · 2025-10-10T08:14:15Z

.github/workflows/main.yml

+    - name: install needed
+      # Not using setup-msys2 `install` option to make it easier to copy/paste
+      run: |
+        pacman --noconfirm -S --needed --overwrite '*' git rust


--overwrite '*' is sus; it seems like a workaround for weird packaging bugs;
I hope we don't need this, so better remove it and add it only if we really need to

krobelus · 2025-10-10T08:14:15Z

share/tools/web_config/webconfig.py


 def runThing():
-    if isMacOS10_12_5_OrLater():
+    if os.environ.get("BROWSER", "") == "true":


yep that looks reasonable;
for as long as we have all these weird workarounds,
we want a reliable way to have system tests disable this.

BTW, we should probably leave out the , "" default argument,
it doesn't add any useful behavior, and it's already obvious
from our use of .get() that a missing variable is not an error.

(As mentioned in #11926 this workaround for macOS 10.12.5 or later,
has been fixed in macOS 10.12.6 according to https://andrewjaffe.net/blog/2017/05/python_bug_hunt/,
I think we can assume all users have upgraded to that patch version
So let's remove this workaround, but that can be in a different PR).

BTW, we should probably leave out the , "" default argument,
it doesn't add any useful behavior, and it's already obvious

I thought get would throw if the variable was not present, but I must have been confused with [...] or the get of some other language. Will remove.

krobelus · 2025-10-10T08:14:15Z

tests/checks/tmux-fish_config.fish

+isolated-tmux capture-pane -p 
+# CHECK: prompt 0> BROWSER=true fish_config
+# CHECK: Web config started at file://{{.*}}.html
+# CHECK: If that doesn't work, try opening http://localhost:8000/{{\w+}}/


This fails for me due to a port number mismatch:

If that doesn't work, try opening http://localhost:8001/9b54d4275031129e9fa88e340623e5f9/

Probably we don't care about the exact port number?

FWIW, I ran the test twice, in two different ways:

# passes ninja -Cbuild test_tmux-fish_config.fish # fails tests/test_driver.py target/debug tests/checks/tmux-fish_config.fish

I hardcoded the number because that way I could just hardcode the bad URL. But it's easy enough to extract it from base_url.

As-is it certainly will fail if you have another instance of fish_config already running (or, of course, if any other server app is on that port)

krobelus · 2025-10-10T08:14:16Z

tests/checks/tmux-fish_config.fish

+
+# Check a bad URL
+wget -q -O /dev/null "http://localhost:8000/invalid_auth/" 2>/dev/null
+or test $status -eq 8


as written, this test statement is useless.
If the test statement fails, nothing happens.
Should follow it up with or echo "unexpected exit code from wget".
That's probably more reliable than testing the exact wget output below.

krobelus · 2025-10-10T08:14:16Z

tests/checks/tmux-fish_config.fish

+
+# Check a good URL
+set -l workspace_root (path resolve -- (status dirname)/../../)
+diff -q "$workspace_root/share/tools/web_config/index.html" (wget -q -O - $base_url 2>/dev/null | psub -f)


krobelus · 2025-10-10T08:18:18Z

I also thought of stdbuf but that expects an actual command, not a shell function.

yeah I could reproduce the same buffering behavior in webconfig.py.
We could change that but I don't see a problem with the current approach.
Just needs small test robustness fixes, then we're good

- Windows allows port reuse under certain conditions. In fish_config case, this allows the signal socket to use the same port as http (e.g. when using MINGW python). This can cause some browsers to access the signal socket rather than the http one (e.g. when connecting using IPv4/"127.0.0.1" instead of IPv6/"::1"). - This is also more efficient since we already know that all ports up to and including the http one are not available Fixes fish-shell#11805

In particular - test that it will return an error if the URL is invalid - that the main page matches the index.html in git - that "Enter" key will exit

Nahor · 2025-10-10T23:17:10Z

Arf! Looks like Busybox's wget has different status codes than GNU's. I tried to add wget to the alpine docker image but it looks like it's a separate process to build those imagea. So for now, I'm doing a quick&dirty check of which version of wget we have and change the status code accordingly.
I believe it's a corner case though (alpine being a very minimal distribution), and that we can assume GNU wget is pre-installed otherwise.

Part of fish-shell#11907

- Windows allows port reuse under certain conditions. In fish_config case, this allows the signal socket to use the same port as http (e.g. when using MINGW python). This can cause some browsers to access the signal socket rather than the http one (e.g. when connecting using IPv4/"127.0.0.1" instead of IPv6/"::1"). - This is also more efficient since we already know that all ports up to and including the http one are not available Fixes fish-shell#11805 Part of fish-shell#11907

Closes fish-shell#11907

In particular - test that it will return an error if the URL is invalid - that the main page matches the index.html in git - that "Enter" key will exit Part of fish-shell#11907

Part of fish-shell#11907

- Windows allows port reuse under certain conditions. In fish_config case, this allows the signal socket to use the same port as http (e.g. when using MINGW python). This can cause some browsers to access the signal socket rather than the http one (e.g. when connecting using IPv4/"127.0.0.1" instead of IPv6/"::1"). - This is also more efficient since we already know that all ports up to and including the http one are not available Fixes fish-shell#11805 Part of fish-shell#11907

Closes fish-shell#11907

In particular - test that it will return an error if the URL is invalid - that the main page matches the index.html in git - that "Enter" key will exit Part of fish-shell#11907

Part of fish-shell#11907

- Windows allows port reuse under certain conditions. In fish_config case, this allows the signal socket to use the same port as http (e.g. when using MINGW python). This can cause some browsers to access the signal socket rather than the http one (e.g. when connecting using IPv4/"127.0.0.1" instead of IPv6/"::1"). - This is also more efficient since we already know that all ports up to and including the http one are not available Fixes fish-shell#11805 Part of fish-shell#11907

Closes fish-shell#11907

In particular - test that it will return an error if the URL is invalid - that the main page matches the index.html in git - that "Enter" key will exit Part of fish-shell#11907

Part of fish-shell#11907

- Windows allows port reuse under certain conditions. In fish_config case, this allows the signal socket to use the same port as http (e.g. when using MINGW python). This can cause some browsers to access the signal socket rather than the http one (e.g. when connecting using IPv4/"127.0.0.1" instead of IPv6/"::1"). - This is also more efficient since we already know that all ports up to and including the http one are not available Fixes fish-shell#11805 Part of fish-shell#11907

Closes fish-shell#11907

In particular - test that it will return an error if the URL is invalid - that the main page matches the index.html in git - that "Enter" key will exit Part of fish-shell#11907

Part of #11907

- Windows allows port reuse under certain conditions. In fish_config case, this allows the signal socket to use the same port as http (e.g. when using MINGW python). This can cause some browsers to access the signal socket rather than the http one (e.g. when connecting using IPv4/"127.0.0.1" instead of IPv6/"::1"). - This is also more efficient since we already know that all ports up to and including the http one are not available Fixes #11805 Part of #11907

In particular - test that it will return an error if the URL is invalid - that the main page matches the index.html in git - that "Enter" key will exit Part of #11907

Berrysoft reviewed Oct 9, 2025

View reviewed changes

Nahor force-pushed the issue_11805 branch from a669a57 to 9745027 Compare October 9, 2025 17:36

krobelus reviewed Oct 9, 2025

View reviewed changes

krobelus approved these changes Oct 9, 2025

View reviewed changes

Nahor force-pushed the issue_11805 branch 3 times, most recently from 8ad4a7a to 18eccd5 Compare October 10, 2025 02:46

krobelus reviewed Oct 10, 2025

View reviewed changes

Nahor added 2 commits October 10, 2025 11:57

Fix build for MSYS2 (missing ulimits)

f86eed6

Nahor force-pushed the issue_11805 branch 2 times, most recently from a4a773f to b1b6b4c Compare October 10, 2025 19:17

Nahor added 2 commits October 10, 2025 16:12

Add test for fish_config in browser modified

88f03c5

In particular - test that it will return an error if the URL is invalid - that the main page matches the index.html in git - that "Enter" key will exit

Add Github action to compile for MSYS2

fe28c0d

Nahor force-pushed the issue_11805 branch from b1b6b4c to fe28c0d Compare October 10, 2025 23:12

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 11, 2025

Fix build for MSYS2 (missing ulimits)

3675110

Part of fish-shell#11907

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 11, 2025

Add Github action to compile for MSYS2

38539e5

Closes fish-shell#11907

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 11, 2025

Fix build for MSYS2 (missing ulimits)

fe3c4ba

Part of fish-shell#11907

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 11, 2025

Add Github action to compile for MSYS2

f64a027

Closes fish-shell#11907

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 11, 2025

Fix build for MSYS2 (missing ulimits)

9fcf24c

Part of fish-shell#11907

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 11, 2025

Add Github action to compile for MSYS2

387da26

Closes fish-shell#11907

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 11, 2025

Fix build for MSYS2 (missing ulimits)

fda35c3

Part of fish-shell#11907

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 11, 2025

Add Github action to compile for MSYS2

8842a1c

Closes fish-shell#11907

krobelus pushed a commit that referenced this pull request Oct 11, 2025

Fix build for MSYS2 (missing ulimits)

8c387c5

Part of #11907

krobelus closed this in a00e6f8 Oct 11, 2025

krobelus pushed a commit that referenced this pull request Oct 11, 2025

Add test for fish_config in browser modified

65332ea

In particular - test that it will return an error if the URL is invalid - that the main page matches the index.html in git - that "Enter" key will exit Part of #11907

Nahor deleted the issue_11805 branch October 11, 2025 17:55

Uh oh!

Fixes for MSYS2 #11907

Fixes for MSYS2 #11907

Uh oh!

Conversation

Nahor commented Oct 7, 2025

TODOs:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Berrysoft commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nahor Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krobelus commented Oct 9, 2025

Uh oh!

krobelus commented Oct 9, 2025 via email

Uh oh!

Nahor commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krobelus commented Oct 10, 2025

Uh oh!

Nahor commented Oct 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Nahor Oct 9, 2025 •

edited

Loading