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.

Sign up for GitHub

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

Jump to bottom

os: implement readdir for darwin and linux #2499

Merged
merged 2 commits into from
Feb 2, 2022
Merged

os: implement readdir for darwin and linux #2499

merged 2 commits into from
Feb 2, 2022

Conversation

dkegel-fastly
Copy link
Contributor

@dkegel-fastly dkegel-fastly commented Jan 8, 2022 •
edited
Loading

Other operating systems are left as an exercise for the reader :-)

@dkegel-fastly dkegel-fastly marked this pull request as draft January 8, 2022 20:50
@dkegel-fastly dkegel-fastly force-pushed the dkegel-os-add-readdir branch 3 times, most recently from f82a567 to 86de1fb Compare January 15, 2022 02:11
@dkegel-fastly dkegel-fastly changed the title os: implement readdir for darwin os: implement readdir for darwin and linux Jan 15, 2022
@dkegel-fastly
Copy link
Contributor Author

dkegel-fastly commented Jan 15, 2022 •
edited
Loading

The i386 problem this ran into in CI may be related to #1906

It can be reproduced with the commands

make
cd tests/testing/pass
GOARCH=386 ../../../build/tinygo test

in this branch, but not in dev.

@dkegel-fastly dkegel-fastly marked this pull request as ready for review January 17, 2022 18:21
dgryski
dgryski approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@dgryski dgryski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Large patch, but almost all of it seems to be from upstream Go. And it has tests, which pass. 👍

@deadprogram
Copy link
Member

Something not working quite right here: https://github.com/tinygo-org/tinygo/runs/4845686957?check_suite_focus=true#step:17:420

tinygo:ld.lld: error: undefined symbol: syscall.seek

@dkegel-fastly
Copy link
Contributor Author

Yes, see #2499 (comment)

The real fix may involves Ayke's fix for #1906

Until then, maybe I could figure out some clever way to exclude the architectures with the problem...?

@dkegel-fastly
Copy link
Contributor Author

Needs a tiny bit more cleanup. Also can't land until the go assembly feature lands. So marking draft again for now.

@dkegel-fastly dkegel-fastly marked this pull request as draft January 17, 2022 23:32
@dkegel-fastly dkegel-fastly force-pushed the dkegel-os-add-readdir branch 2 times, most recently from 13185f8 to daf1d2a Compare January 22, 2022 22:01
@dkegel-fastly
Copy link
Contributor Author

Rebased atop #2571 for the seek fix

@dkegel-fastly dkegel-fastly force-pushed the dkegel-os-add-readdir branch 3 times, most recently from baff248 to 3ea7a36 Compare January 22, 2022 23:59
@dkegel-fastly dkegel-fastly mentioned this pull request Jan 23, 2022
Closed
@dkegel-fastly dkegel-fastly marked this pull request as ready for review January 23, 2022 02:24
@dkegel-fastly
Copy link
Contributor Author

#2571 should land first, but otherwise this is ready for review.

@dkegel-fastly dkegel-fastly force-pushed the dkegel-os-add-readdir branch 3 times, most recently from 57e9d46 to 6902f91 Compare January 23, 2022 23:17
dkegel-fastly
dkegel-fastly commented Jan 23, 2022
View reviewed changes
src/os/dir_darwin.go Outdated Show resolved Hide resolved
@dankegel
Copy link
Contributor

dankegel commented Jan 24, 2022 •
edited by dkegel-fastly
Loading

I think this is ready, now that I've tested it with io/fs and fixed the problem this found on Darwin.

@dkegel-fastly dkegel-fastly added this to the v0.22.0 milestone Jan 24, 2022
@dkegel-fastly dkegel-fastly mentioned this pull request Jan 24, 2022
Merged
@dkegel-fastly dkegel-fastly removed this from the v0.22.0 milestone Jan 24, 2022
@dkegel-fastly
os: implement readdir for darwin and linux
16cb8b5 
readdir is disabled on linux for 386 and arm until syscall.seek is implemented there.

windows is hard, so leaving that for later.

File src/os/dir_other_go115.go can be deleted when we drop support for go 1.15.

Also adds TestReadNonDir, which was helpful while debugging.
@dkegel-fastly
os: add DirFS, which is used by many programs to access readdir.
e9fd684 
It's wafer-thin :-)

Includes smoke test from upstream.

TODO: once t.TempDir is implemented, add io/fs to the list of standard library tests to run; that's a better test.
dgryski
dgryski approved these changes Feb 2, 2022
View reviewed changes
Copy link
Member

@dgryski dgryski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@deadprogram
Copy link
Member

Thanks @dkegel-fastly and @dgryski now merging.

@deadprogram deadprogram merged commit 98a6ed8 into tinygo-org:dev Feb 2, 2022
@QuLogic QuLogic mentioned this pull request Feb 6, 2022
Closed
QuLogic
QuLogic reviewed Feb 6, 2022
View reviewed changes
src/os/file_other.go
name string
}

func NewFile(fd FileHandle, name string) *File {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This differs from Go in taking an interface for fd instead of a uintptr, which seems to break if passing, e.g., file descriptor 3 as done in Go 1.18's fuzzing code (cf. #2515)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC that was taken straight from upstream? Maybe upstream has changed, or maybe I'm misremembering.

Either way, please file a bug and we can sort it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's been that way for 10 years, it appears. I'll just open a PR as I've already made the changes locally.

@dkegel-fastly dkegel-fastly mentioned this pull request Feb 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@dgryski dgryski dgryski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dkegel-fastly @deadprogram @dankegel @QuLogic @dgryski