My
$ sudo docker/docker_run_tests.sh --shell-after docker/jammy-asan.Dockerfile
shows a lot of complaints about
Direct leak of 60 byte(s) in 1 object(s) allocated from:
because some unit tests call reader_init() and reader_deinit(). Work around
this by initializing this value only once. AFAICT, OnceCell is async-signal
safe (unlike Mutex), although I don't think documentation promises that.
It doesn't feel great to change implementation code to accomodate tests but
I think for this specific issue that's what we usually do. Alternatively,
we could add to lsan_suppressions.txt.
If cargo ever wants to write to "target/man", it would collide with our
use of this path. Let's make this less likely by prefixing the name with
"fish-". This also makes it more obvious that this is fish's invention.
(Note: this commit should technically have preceded the "Fix config paths
for disjoint build-dirs and in-tree installs" one, to make that one easier
to follow, but I wasn't 100% sure if this commit is right.)
From https://doc.rust-lang.org/cargo/reference/environment-variables.html
> OUT_DIR — If the package has a build script, this is set to the folder
> where the build script should place its output. See below for more
> information. (Only set during compilation.)
so OUT_DIR is something like "target/debug/build/fish-41da27d587f48978".
Whenever build.rs is re-run, we get a new one.
I don't think we need this flexibility anywhere. It wouldn't protect
concurrent "cargo test" from interfering with each other - that's handled
by a file lock taken by Cargo.
Use "target/" instead (or CMAKE_BINARY_DIR if set).
Namespace the files better, so we don't create weird paths like
target/test/complete_test/...
target/fish_root/
With the upcoming tests/checks/gettext.fish test from #11583, my
sudo docker/docker_run_tests.sh --shell-after docker/focal.Dockerfile
fails writing to "po/template.po" because "/fish-source" is mounted as
read-only. (There should be no need for tests to write to the source tree.)
Since commit 6239cba1e4 (Add dry-run mode to update_translations.fish,
2025-05-30), "build_tools/update_translations.fish" always removes that
template file when done, even without "--dry-run".
I'm not sure if we still have a need for keeping around "po/template.po".
To add a new translation, you can run "build_tools/update_translations
po/xy.po". It could serve as a cache but that would only work if we integrated
it into a build system.
Move it to /tmp, fixing the docker tests.
Use test_driver directly instead of CMake in the docker tests.
Deal with the read-only "/fish-source" by exporting
"CARGO_TARGET_DIR=$HOME/fish-build". It seems correct to also inject this
environment variable into the interactive debugging shells. Add some logging
to make this override more obvious to the user.
Adopt "build_tools/check.sh", because that defines the full set of checks
that we (eventually) want to run in CI.
In particular, this will also run "tests/checks/po-files-up-to-date.fish"
which "cargo b && cargo t && tests/test_driver.py" does not, due to the
REQUIRES clause.
Since most docker images have some lints/warnings today, disable those for
now. Use "docker_run_tests.sh --lint" to override. The default may be changed
in future.
Commit 89282fd9bc (Use CARGO_MANIFEST_DIR to see if we're running from
build dir, 2024-01-20) did
-if exec_path.starts_with(OUT_DIR)
+if exec_path.starts_with(CARGO_MANIFEST_DIR)
where OUT_DIR is the cmake build directory ("./build")
and CARGO_MANIFEST_DIR is our top level source tree.
This allowed "target/debug/fish" to work, but it broke
1. CMake build directories outside the source tree, e.g. "docker/docker_run_tests.sh".
Those incorrectly fall back to the compiled-in-path (/usr/local/share etc)
2. Installations iside the source tree, e.g.
"mkdir build && cd build && cmake .. -DCMAKE_INSTALL_PREFIX=$PWD/../install".
These installations incorrectly use "share/" etc. from the source tree.
Fix this by
1. respecting the CMake-specific FISH_BUILD_DIR, ad
2. if that's not set, use $CARGO_MANIFEST_DIR/target
We have a mixture of 2 and 4 space indent.
4 benchmarks/driver.sh
2 build_tools/check.sh
4 build_tools/git_version_gen.sh
4 build_tools/mac_notarize.sh
2 build_tools/make_pkg.sh
2 build_tools/make_tarball.sh
2 build_tools/make_vendor_tarball.sh
4 docker/docker_run_tests.sh
4 osx/install.sh
2 tests/test_functions/sphinx-shared.sh
Our editorconfig file specifies 2, with no explicit reason.
Our fish and Python scripts use 4, so let's use that.
Commit 5c0fddae70 (Refactor history flushing, 2025-03-28) made three changes:
1. call fsync() when we are finished writing the history file.
2. when appending to (as opposed to vacuuming) history, call write(2)
(followed by flush() and sync()) for each item. Previously, we'd only
call write(2) if our 64k buffer was full, or after processing the last
history item.
3. actually check the return value of flush() (which would retry when flushing
fails -- but std::fs::File::flush() never fails!).
The motivation was to potentially fix#10300 which didn't succeed (see
https://github.com/fish-shell/fish-shell/issues/10300#issuecomment-2876718382).
As for 1 and 2, I don't think the way we use fsync really helps, and flushing
eagerly should not make a difference.
As for 3, there are some explanations in comments, commit message and a [PR
comment](https://github.com/fish-shell/fish-shell/pull/11330#discussion_r2020171339).
To summarize, 5c0fddae70 wants to address the scenario where file.flush()
fails. Prior to that commit we would ostensibly carry on with a corrupted
"first_unwritten_new_item_index" (corrupted because it doesn't match what's
written to disk), which can cause various issues. However this doesn't
ever happen because std::fs::File::flush() never fails because it doesn't
do anything -- std::fs::File::write() does not buffer writes, it always
delegates to write(2).
There can definitely be scenarios like the one described in
https://github.com/fish-shell/fish-shell/pull/11330#discussion_r2020171339
where the disk is full. In that case, either write(2) fails, which we
already check. Or close(3p) fails with EIO, which we have never checked. We
should probably check that.
Undo all three changes for now.
Closes#11495
Remove dependency on CTest. Parallel execution is handled by `test_driver.py`
internally now, so CTest is no longer relevant for performance.
This also removes CMake targets for single tests. As a replacement,
`test_driver.py` can be called directly with the path to the build directory as
the first argument and the path to the desired test as the second argument.
Ensuring that the executables in the build directory are up to date needs to be
done separately.
For a pure cargo build, an example of running a single test would be:
`cargo b && tests/test_driver.py target/debug tests/checks/abbr.fish`
The recommended way of running tests is `build_tools/check.sh`, which runs more
extensive tests and does not depend on CMake. That script does not work in CI
yet, so CMake testing is retained for now.
Update CI config to use the new `FISH_TEST_MAX_CONCURRENCY`.
Also update the FreeBSD version, since the previous one is outdated and does not
support the semaphore logic in `test_driver.py`.
`mmap` should fail when the length argument is 0. Checking this in advance
allows returning early, without performing unnecessary syscalls.
This also fixes an issue observed on FreeBSD 13.2 where `mmap` does not always
fail when the length is 0, resulting in the `assert!(len > 0)` in
`MmapRegion::new` failing.
https://github.com/fish-shell/fish-shell/issues/11595
Commit cd3da62d24 (fix(completion): unescape strings for __fish_complete_list,
2024-09-17) bravely addressed an issue that exists in a lot of completions.
It did so only for __fish_complete_list. Fair enough.
Unfortunately it unescaped more than just "$(commandline -t)".
This causes the problem described at
https://github.com/fish-shell/fish-shell/issues/11508#issuecomment-2889088934
where completion descriptions containing a backslash followed by "n" are
interpreted as newlines, breaking the completion parser. Fix that.
Given a command line like
foo --foo=bar=baz=qux\=argument
(the behavior is the same if '=' is substituted with ':').
fish completes arguments starting from the last unescaped separator, i.e.
foo --foo=bar=baz=qux\=argument
^
__fish_complete_list provides completions like
printf %s\n (commandline -t)(printf %s\n choice1 choice2 ...)
This means that completions include the "--foo=bar=baz=" prefix.
This is wrong. This wasn't a problem until commit f9febba (Fix replacing
completions with a -foo prefix, 2024-12-14), because prior to that, replacing
completions would replace the entire token.
This made it too hard to writ ecompletions like
complete -c foo -s s -l long -xa "hello-world goodbye-friend"
that would work with "foo --long fri" as well as "foo --long=frie".
Replacing the entire token would only work if the completion included that
prefix, but the above command is supposed to just work.
So f9febba made us replace only the part after the separator.
Unfortunately that caused the earlier problem. Work around this. The change
is not pretty, but it's a compromise until we have a better way of telling
which character fish considers to be the separator.
Fixes#11508
The main purpose of this is avoiding timeouts in CI.
Passing `--max-concurrency=n` to `test_driver.py` will result in at most `n`
tests running concurrently, where `n` is a positive integer.
Not specifying the argument preserves the old behavior of running all tests
concurrently without a limit.
This was missed in the Rust port - C++ had statfs for MNT_LOCAL and not statvfs.
The effect of this is that fish never thought its filesystem was local on macOS
or BSDs (Linux was OK). This caused history race tests to fail, and also could
in rare cases result in history items being dropped with multiple concurrent
sessions.
This fixes the history race tests under macOS and FreeBSD - we weren't locking
because we thought the history was a remote file.
Within a linked worktree, `$GIT_DIR` and `$GIT_COMMON_DIR` have different
values (see [git-worktree docs](https://git-scm.com/docs/git-worktree#_details)).
The two serve different purposes, in case of stashes `$GIT_COMMON_DIR`
should be used, this way stash detection in the git prompt works also
when inside a `git worktree`.
Closes#11591
Systems like NixOS might not have "git-receive-pack" or any other "git-*"
executable in in $PATH -- instead they patch git to use absolute paths.
This is weird. But no reason for us to fail. Silence the error.
Fixes#11590
Eliminates some code duplication between the two different saving
implementations. These changes are based on
https://github.com/fish-shell/fish-shell/pull/11492#discussion_r2149438316.
One extra change is included here, namely the early return on an empty file
name, indicating private mode. Without this, `history_path.unwrap()` fails in
some tests. Returning early is probably what we want in such situations anyway.
The cached information might become outdated. It is important that all fish
processes use the same mutual exclusion logic (either `flock`-based or the
fallback), because the two methods do not provide mutual exclusion from one
another.
Avoiding caching makes the behavior independent on previous system states,
resulting in fish instances performing file operations at the same time to use
the same locking logic.
More detailed discussion in
https://github.com/fish-shell/fish-shell/pull/11492#discussion_r2134543447
If arguments always have the same value they do not need to be arguments.
For `HistoryImpl::add`, the comments are incorrect (assuming they should
indicate the value of the argument), since both arguments can be true or false
independently.
`History::add` is only called at one location in a test with a constant value of
`false` for `pending`. This might mean that the parameter could be deleted, or
maybe even the entire function, if testing can work without it.
Both history files and universal variables files are accessed by multiple
processes, which need a way to synchronize their accesses.
This synchronization logic is mixed in with the logic for reading and updating
the files' contents, which results in messy code, duplicated locking
implementations, and inconsistencies.
Moreover, the existing implementations are flawed which has resulted in file
corruption (e.g. https://github.com/fish-shell/fish-shell/issues/10300).
The new approach separates the synchronization logic from the rest.
There are two approaches to synchronization.
- The primary one is using `flock(2)` to lock the directory containing the file
which requires synchronized access. We do not lock the file holding the data
directly because this file might be replaced, which can result in locking
succeeding when it should block. Locking the directory solves this problem.
To avoid inconsistent file states, changes are first written to a temporary
file, which is then renamed into place while still holding the lock.
- In some situations `flock` locks are unavailable or take a long time. This
mostly applies to remote file systems. If we think that a directory is located
on a remote file system, we do not attempt to use `flock`.
As a fallback, we have a lockless approach, which uses file metadata (device,
inode, size, ctime, mtime) to identify file versions.
We then read from the file, write the updated data to a temporary file,
check if the file id for the path is the same as before we started reading,
and if so we rename the temporary file such that it replaces the old file.
Note that races are possible between the file id check and the rename syscall.
If we detect a file id mismatch, we retry, up to a predetermined number of
attempts.
The operations which should be performed are passed to the functions handling
synchronization as `Fn`s. Because we might have to run these operations
repeatedly when retrying, they should be executable arbitrarily often without
causing side-effects relevant to the program. This requires some changes to
functions accessing these files. In many cases, they have to work with
non-mutable references, which requires that they return the data which should be
updated in the program state, instead of directly assigning to the appropriate
location.
Locking via `O_EXLOCK`, which was used for the universal variable file, is no
longer supported. That version of locking locks the file operated on directly,
instead of its directory. According to the man pages of {Open,Free,Net}BSD
and macOS, these locks have flock semantics. So if flock is available, we can
use it, and if it is not, the `O_EXLOCK` flag does not help.
This is preparatory work for refactoring the file synchronization approach.
`read_exact` will fail if the file length does not match the expected one, which
means zero padding is useless. (On any reasonable OS it was also useless before,
because anonymous memory mapping would be zero-pages.)
Use `std::io::Result` instead of `Option` as the return type where appropriate,
to allow for better-informed error handling.
Remove explicit checks about the length to be mapped being 0.
From `mmap(3)`:
> If len is zero, mmap() shall fail and no mapping shall be established.
