Skip to content

Conversation

codefromthecrypt
Copy link
Contributor

No description provided.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

LGTM. Actually something I wanted to do a while ago but never got around doing it.

@aykevl
Copy link
Member

aykevl commented Sep 8, 2022

The failure here looks suspiciously like the one I saw when I tried Go 1.19. I suspect a bugfix causes the test to fail. I'll need to investigate this.

@aykevl
Copy link
Member

aykevl commented Sep 8, 2022

Looks like this is caused by golang/go@315e80d. Specifically, it is a stack overflow. I suggest removing io/fs from the tested packages because it requires a stack that's just far too large for TinyGo to support.

@dgryski @dkegel-fastly @deadprogram opinions on this removal?

@dgryski
Copy link
Member

dgryski commented Sep 8, 2022

SGTM. We want to keep CI passing.

@deadprogram
Copy link
Member

tinygo-org/tinyfs#6 where @bgould mentions wanting to use this package for tinyfs.

@dankegel
Copy link
Contributor

dankegel commented Sep 8, 2022

Can we raise the stack size on Linux/Mac/windows and keep testing Io/fs there?

@aykevl
Copy link
Member

aykevl commented Sep 8, 2022

It starts to work at 6MB on linux/amd64, which to me seems like an unreasonably large stack size per goroutine. Remember that TinyGo doesn't support growing the goroutine stack.

@dkegel-fastly
Copy link
Contributor

How about adding a stack size commandline option? Then we could keep testing io/fs without burdening the world with big stacks...

@aykevl
Copy link
Member

aykevl commented Sep 14, 2022

How about adding a stack size commandline option?

Yeah, sounds reasonable: #3159
It's less clear to me how it can be integrated in the Makefile, I guess we could add a new tinygo test -stack-size=6MB io/fs command. Or we'd have to run all tests with a 6MB stack, which I'm not sure is a good idea either.
Perhaps a better solution is a way to skip just that particular test (TestCVE202230630), because it's written in a way that is incompatible with TinyGo. Not sure how to do that either.

@codefromthecrypt
Copy link
Contributor Author

@aykevl

It's less clear to me how it can be integrated in the Makefile

could a heavy or slow tag help? It is fairly common for projects to have slow, bench or otherwise code in the makefile, just only run in CI to keep dev flow faster. ex make tinygo-test.slow

FYI I'm keeping the comments running on this topic despite not being related to go versions, as they are here anyway. If we don't come up with a way out before this merges, we should fork an issue to make sure things work smooth.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

I tried a workaround to help middle-ground approach.

@codefromthecrypt
Copy link
Contributor Author

looks like a green. feel free to squash on my behalf if this is a go @deadprogram etc!

Copy link
Contributor

@dkegel-fastly dkegel-fastly left a comment

Choose a reason for hiding this comment

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

LGTM!

@codefromthecrypt
Copy link
Contributor Author

anyone has my permission to squash or obviate this with another PR if the solution isn't good enough. I'll be away for a few days.

@deadprogram
Copy link
Member

Thanks very much for fixing this @codefromthecrypt now going to squash/merge.

@deadprogram deadprogram merged commit 25f6be1 into tinygo-org:dev Sep 24, 2022
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.

6 participants