implements lstat and fixes inode stat on windows go 1.20 #1168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
codefromthecrypt
force-pushed
the
lstat
branch
3 times, most recently
from
37bf7e5 to
2f9ae52
Compare
codefromthecrypt
changed the title
.github/workflows/integration.yaml
Outdated
| @@ -18,7 +18,7 @@ defaults: | |||
| shell: bash | |||
|
|
|||
| env: # Update this prior to requiring a higher minor version in go.mod | |||
| GO_VERSION: "1.19" # 1.xx == latest patch of 1.xx | |||
| GO_VERSION: "1.20" # 1.xx == latest patch of 1.xx | |||
There was a problem hiding this comment.
this was supposed to be latest, and also it is important because it means windows works fine.
| @@ -143,11 +143,22 @@ func (jsfsLstat) invoke(ctx context.Context, mod api.Module, args ...interface{} | |||
| path := args[0].(string) | |||
| callback := args[1].(funcWrapper) | |||
|
|
|||
| lstat, err := syscallStat(mod, path) // TODO switch to lstat syscall | |||
| lstat, err := syscallLstat(mod, path) | |||
There was a problem hiding this comment.
lstat is needed on GOOS=js because reading a directory involves fetching the dir names then an lstat per entry. This can't use stat because stat follows links.
| var stat1 Stat_t | ||
| require.NoError(t, StatFile(d, &stat1)) | ||
| requireDirectoryDevIno(t, stat1) |
There was a problem hiding this comment.
missing test. before we had no idea directories weren't returning inodes on windows
|
zig windows test is taking forever now. will turn on host logging to see what's going on. |
mathetake
reviewed
Feb 26, 2023
|
looks like windows is dying after this. I wonder if we can make sure that zig exits faster vs hanging? wdyt @mathetake? |
|
added |
codefromthecrypt
force-pushed
the
lstat
branch
from
a21edb1 to
fbdfa27
Compare
|
ok this is now rebased on the same zig version that passes on main |
|
hostlogging isn't really viable in zig since the tests can't be skipped at runtime. the compiled stdlib test wasm isn't tainted per PR, which means it runs all of them. the logs here are 2.3GB for example! |
|
ok something is looping in $ cat -n ~/Downloads/logs |grep ' test.'
--snip--
1904 2023-02-27T01:23:17.1788043Z 961/2123 test.Dir.readLink... ==> wasi_snapshot_preview1.path_create_directory(fd=3,path=zig-cache) |
|
yep something is calling path_unlink_file and path_open in a loop |
|
here's darwin |
|
@mathetake so this seems the root cause is our implementation is struggling to delete a symlink on windows. I have to run for a couple hours in case you have time to try this in this PR or a separate one. here's the start of the loop on windows. I think possibly the EISDIR is misleading (as it is a link) or something else amuck when trying to path_unlink_file symlink2 |
|
#1172 here's a repro |
codefromthecrypt
force-pushed
the
lstat
branch
from
e5518e3 to
ddfa784
Compare
This implements platform.Lstat and uses it in GOOS=js. Notably, directory listings need to run lstat on their entries to get the correct inodes back. In GOOS=js, directories are a fan-out of names, then lstat. This also fixes stat for inodes on directories. We were missing a test so we didn't know it was broken on windows. The approach used now is reliable on go 1.20, and we should suggest anyone using windows to compile with go 1.20. Signed-off-by: Takeshi Yoneda <[email protected]> Co-authored-by: Adrian Cole <[email protected]>
codefromthecrypt
force-pushed
the
lstat
branch
from
7e01b82 to
a554dd0
Compare
evacchi
approved these changes
Feb 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements platform.Lstat and uses it in GOOS=js. Notably, directory listings need to run lstat on their entries to get the correct inodes back. In GOOS=js, directories are a fan-out of names, then lstat.
This also fixes stat for inodes on directories. We were missing a test so we didn't know it was broken on windows. The approach used now is reliable on go 1.20, and we should suggest anyone using windows to compile with go 1.20.