-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Clearer separation of WASI POSIX support with (or without) wasi-libc #20991
Conversation
The problem here is going to be the fact that we are no longer interested in maintaining a POSIX compatibility layer in the standard library - see #6600. This has 2 main implications for this PR:
See also #19614. |
Hmm, I can see the argument is for not making up a POSIX layer on target that don't have one. But given that WASI doesn't try to be POSIX (though they justify a lot of their APIs in terms of a POSIX relationship), that implies the Zig POSIX-via-WASI wrapper should completely go away. I was under the impression that the goal was to keep the Zig-impl-of-POSIX-on-WASI running (at least in the short term). Did I misinterpret #20845 (review)? (My interpretation is that std.posix.* should be a best-effort and implement POSIXy APIs for systems where that isn't too far out of whack, so for WASI without libc we get POSIX file open/read/write, but no chown/chmod/mmap, etc) What you're suggesting, concretely, is that instead of making up a
I understand your suggestion to be removing all the Can you maybe squint at this change and see it as a cleanup step in preparation for that deletion? (It helps clearly separate the existing Zig POSIX on wasi-without-libc?) OTOH:
The last comment from our BDFL is "man. wasi-libc kinda sucks. how about we provide libc via zig code instead?" Isn't a "libc via zig code" exactly what the std.posix implementation in the wasi-without-libc path is providing? |
Well, that would indeed be my position. 🙂 I want
I do think that it should at least be kept to the same level of "running" as it is today, until we settle on a final decision regarding the future of
Maybe I'm completely off, but I interpret Andrew's comment there to mean exactly what I'm saying:
POSIX But again, to be clear, I'm only saying this for new additions to
My suggestion is that any such blocks that already exist, and work, should stay. What I am suggesting is that I'm not sure we want to add more such blocks going forward if they require us to make stuff up about the underlying
I'm not suggesting that you should do all that work for existing code paths. 🙂 The code paths that go through What I am suggesting is that this should be done for any code paths that aren't currently working for wasi-without-libc.
That's up to Andrew. I personally wouldn't lose sleep over the changes here because my ideal world means pulling
I can only guess here, but I took it to mean something like ziglibc, not necessarily part of |
a505934
to
a630267
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I think this is taking things in the wrong direction
Thanks for taking the time and the detailed feedback, I think I understand where you are coming from, and I think that's actually pretty consistent with my understanding too.
I think I can make the case that, other than the types at the top (in the These types in This PR also fixes a lot of tests to run on Wasi with or without libc, there are relatively few spots where tests are covering wasi-without-libc specifically. So, I think you can look at this PR as improving the wasi-with-libc support in Zig by (1) improving the test coverage (2) fixing some bugs in the |
d4b0601
to
c045747
Compare
c045747
to
516be96
Compare
Make a clearer separation between the Posix defines for WASI without a libc (the `system` defined in posix.zig) and the Posix defines for WASI with a libc (which should mirror the Musl-based wasi-libc implementation, and are defined in c.zig). Make WASI `posix.O` look more POSIX-y when compiling without libc. These structures and constants do not need to mirror the with-libc Wasi API (e.g., can use `.ACCMODE`). This removes some Wasi-specific code in Dir.zig and posix.zig by making the structure use names consistent with other platforms. Define `.S`, `.timespec`, and `.AT` structs for Wasi-without-libc in posix.zig. These are based on the wasi-with-libc structures, but cleaned up to remove cruft like padding or wholly unsupported fields. Fix `.S` and `.AT` constants on wasi-with-libc (defined in c.zig) to match wasi-libc headers. Define `mode_t` type as `void` for wasi targets because file permissions are not supported via the Posix APIS on these platforms. Use `mode_t` as the parameter type in Zig's mkdir\* and openat\* wrappers. Re-enable a lot of tests that have accumulated stale not-on-wasi guards, and simplify some tests (e.g., dropping unnecessary absolute paths) so they can run on wasi targets.
To be consistent with the Wasi mode_t, use 'void'.
Add `unique_fname` for creating unique-name files when testing in the default CWD (though tests should still prefer tmpDir()). Enable the tests disabled because this is racy (ziglang#14968). Add some WASI-specific behavior handling. Other cleanups of the tests: * use `realpathAlloc()`to get fullpath into tmpDir() instances * use `testing.allocator` where that simplifies things (vs. manual ArenaAllocator for 1 or 2 allocs) * Trust `TmpDir.cleanup()` to clean up sub-trees * Remove some unnecessary absolute paths (to enable WASI tests) * Drop some no-longer necessary `[_][]const u8` explicit types * Put file cleanups into `defer` Fixes ziglang#14968
Add a test for fstat{,at} with symlinks (for everyone, not just wasi) Fix the wasi-libc Stat.mode field, and add the expected accessors to the S struct. Fixes ziglang#20890
Point to the correct header files where the wasi-libc constants are defined that the Zig lib/std/c.zig is replicating. Zig's current copy of wasi-libc has a bug in the file-mode constants. Both S_IFIFO and S_IFSOCK are 0xc000, but S_IFIFO should be 0x1000. See WebAssembly/wasi-libc#463.
c2eac3f
to
3be1ed0
Compare
I'll send out the WASI bug fixes in separate, smaller changes. |
Make a clearer separation between the Posix defines for WASI without a libc (the
system
defined in posix.zig) and the Posix defines for WASI with a libc (which should mirror the Musl-based wasi-libc implementation, and are defined in c.zig).This PR is mostly focused on cleanly separating the
std.posix
andstd.c
WASI implementations, disentangling types and structures where that makes sense.For context, here are the existing ways to access system services from Zig when compiling against a wasi target (e.g.,
-target wasm32-wasi
or-target wasm32-wasi -lc
):std.os.wasi
). These are non-POSIX APIs that provide access to APIs outside the WASM sandbox. Zig provides the "preview 1" version of the WASI API.std.posix
API implemented directly on top of the WASI APIs. This is Zig's implementation of a subset of the POSIX API.std.c
API as implemented by wasi-libc (a fork of the Musl C Library for WASI), when built with-lc
, this provides a richer implementation of the POSIX API implemented by wasi-libc (customized from musl libc) on the WASI API.File.zig
,Dir.zig
, etc). These Zig-specific APIs are often built onstd.os.wasi
, but sometimes go throughstd.posix
.Summary of Changes
Make WASI
posix.O
look more POSIX-y when compiling without libc. These structures and constants do not need to mirror with with-libc Wasi API. For example, use of.ACCMODE
. This removes a lot of Wasi-specific code in Dir.zig and posix.zig by making the structure use names consistent with other platforms. Define.S
,.timespec
, and.AT
structs for wasi-without-libc in posix.zig. These are based on the wasi-with-libc structures, but cleaned up to remove cruft like padding or wholly unsupported fields.Fix
.S
and.AT
constants on wasi-with-libc (defined in c.zig) to match wasi-libc headers.Define
mode_t
type asvoid
for wasi targets (and Windows targets) because file permissions are not supported via the Posix APIS on these platforms. Usemode_t
as the parameter type on Zig's mkdir* and openat* signatures (instead ofu32
).Note
mode_t
is used both as a parameter for open/chmod/chown type functions (encoding user permissions) and also used as a property ofstruct stat
results, which additionally encode file type (link, directory, regular, etc). These should probably be separate types?std.c.makeOFlags()
is an attempt to make initializingstd.posix.O
flags with or without the .ACCMODE simpler. The wasi-with-libc O flags cannot support ACCMODE due to the layout of the flags in wasi-libc.The with-libc WASI
AT
struct needs to mirror the wasi-libc#defines
. Fixes #20890Enable a lot of tests that have accumulated stale not-on-wasi guards, and simplify some tests (e.g., dropping unnecessary absolute paths) so they can run on both of the wasi targets. Also, factors out
supports_chmod
andsupports_absolute_paths
, etc flags in tests to make the skip-this-test logic more readable.Zig's Posix-on-Wasi wrapper defines the current working directory to be
.
(from the--dir=.
argument to wasmtime). However, wasi-libc defines the current working directory to be/
, which makes a bit more sense, as the wasi code is effectively chroot-ed into the given directory. Perhaps Zig should follow wasi-libc's lead here? (I'm not sure it makes any difference anywhere, yet.)Clean up of test cases:
realpathAlloc()
to get the full path of tmpDir() instancestesting.allocator
where that simplifies things (vs. manual ArenaAllocator for 1 or 2 allocations)unique_fname
in posix.zig for creating unique-name files when testing in the default cwd (prefer tmpDir()). Fixes unit tests that write files to cwd have racy failures #14968[_][]const u8
explicit typesdefer