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

Redo: Handle execveZ failures in Command.zig tests #3176

Merged
merged 3 commits into from
Dec 29, 2024

Conversation

anund
Copy link
Contributor

@anund anund commented Dec 26, 2024

#3130 do over. Github seems to have lost track of the fork status for the previous work. (perhaps permissions?). Rebased the PR to main as I can't see why it could possibly fail.

Previous description Bit of a rabbit hole came up while trying to package ghostty for nixos. zig build test would just hang when run as part of checkPhase in a nix build. (rather than the nix develop ... command run in CI).

Here's what I understand so far:

/usr/bin/env does not exist in a nix build sandbox. This only works as a test binary when running in nix develop as it shares its environment with the user where this compatibility crutch exists.
When executing Command.zig tests that reference /usr/bin/env the eventual call to fork+execevZ will duplicate the running test process as execevZ returns rather than dissapearing into the new exec'd process. Duplicating a test process via fork does unexepected things. zig build test will hang for <reasons?>. A test binary created via -Demit-test-exe will run two copies of the test suite producing two different failure outputs for the same test. (or ~4 copies of the test framework, one extra for each test that fails this way)

/bin/sh can be used and an alternative test target. It isn't amazing as it's relying on stdenv creating /bin/sh and it just existing on user systems. Other alternatives I can think of would require build flags or some sort of contract with packaging around what binary will exist for the Command.zig tests.

@anund
Copy link
Contributor Author

anund commented Dec 26, 2024

I can't see how this fails in CI.

Ci appears to be using the right commits, no stale grabs of commits. But the fork is now busted post public.

But I can trigger CI again with the new, public?, fork.

@auscompgeek
Copy link

/bin is a symlink to /usr/bin on many Linux distros these days, including Debian/Ubuntu by default.

@anund anund force-pushed the handle_execveZ_failures branch from 40edeec to 7aaedd6 Compare December 26, 2024 23:49
Duplicating a test process via fork does unexepected things.
zig build test will hang
A test binary created via -Demit-test-exe will run 2 copies of the test suite
@anund anund force-pushed the handle_execveZ_failures branch from 7aaedd6 to b2cb80d Compare December 27, 2024 00:09
@anund anund changed the title Redo: Handle execve z failures Redo: Handle execveZ failures in Command.zig tests Dec 27, 2024
The `Command.zig` tests reach outside the local source tree and look for
files on the host os machine. This introduces some portability issues
for the tests.

The nix build sandbox doesn't include `/usr/bin/env` making it error out
when `zig build test` runs `Command.zig` tests as part of a `nix build`.
Current ci and local development relies on `nix develop` sharing a host os
file system that includes `/usr/bin/env`.

Turns out `/tmp` and `/bin/sh` are available in the build sandbox in
nix so we swap these in to enable nixpkg builds to include testing
ghostty as part of any update cycle.
@anund anund force-pushed the handle_execveZ_failures branch from 49d88b9 to 600eea0 Compare December 27, 2024 06:07
@anund anund mentioned this pull request Dec 27, 2024
13 tasks
@anund
Copy link
Contributor Author

anund commented Dec 27, 2024

Fixed now for nix build. /tmp being /private/tmp gives enough room to make ci/nix build correctly.

@anund
Copy link
Contributor Author

anund commented Dec 29, 2024

@jcollie car to have a second look as the previous PR needed to be closed?

@mitchellh mitchellh merged commit 65a1c0d into ghostty-org:main Dec 29, 2024
17 checks passed
@github-actions github-actions bot added this to the 1.0.1 milestone Dec 29, 2024
@anund anund deleted the handle_execveZ_failures branch January 7, 2025 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants