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

Add test coroutine #70

Closed
wants to merge 3 commits into from
Closed

Conversation

Pavel-Durov
Copy link
Contributor

Related Issue: #58

@Pavel-Durov
Copy link
Contributor Author

We still can't run CI validation with the latest YK
Failed buildbot job - https://ci.soft-dev.org/#/builders/1/builds/2527

@vext01
Copy link
Contributor

vext01 commented Sep 25, 2023

The issue is still open, so how comes we are re-enabling this test?

@Pavel-Durov
Copy link
Contributor Author

The issue is still open because I don't want to close it before I can validate tests with buildbot.
I've tested coroutine.lua myself and it works with try_repeat 1000.

@vext01
Copy link
Contributor

vext01 commented Sep 25, 2023

Was there any change that you'd expect to have fixed that test?

@Pavel-Durov
Copy link
Contributor Author

Yes, this commit: b931338

@vext01
Copy link
Contributor

vext01 commented Sep 25, 2023

bors r+

bors bot added a commit that referenced this pull request Sep 25, 2023
70: Add test coroutine r=vext01 a=Pavel-Durov

Related Issue: #58


Co-authored-by: Pavel Durov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 25, 2023

Build failed:

@vext01
Copy link
Contributor

vext01 commented Sep 27, 2023

What should we do with this?

@Pavel-Durov
Copy link
Contributor Author

good question :)
I don't think we can merge any PRs in yklua since breaking changes in YK.
Last working version: 4a955668d5a6647c5de8f043314983f057a5039e (Sep 15, 2023)

@Pavel-Durov
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 16, 2023
@bors
Copy link
Contributor

bors bot commented Oct 16, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@Pavel-Durov
Copy link
Contributor Author

Bors CI succeeded I guess we can merge it then!? @vext01

@vext01
Copy link
Contributor

vext01 commented Oct 16, 2023

I think so.

Any squashing to do?

@Pavel-Durov
Copy link
Contributor Author

No squashing needed :)

@vext01
Copy link
Contributor

vext01 commented Oct 16, 2023

Great!

bors r+

bors bot added a commit that referenced this pull request Oct 16, 2023
70: Add test coroutine r=vext01 a=Pavel-Durov

Related Issue: #58


Co-authored-by: Pavel Durov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 16, 2023

Build failed:

@vext01
Copy link
Contributor

vext01 commented Oct 16, 2023

16:19:38 thread '<unnamed>' panicked at ykrt/src/mt.rs:469:21:
16:19:38 assertion failed: sidetrace.is_none() ||
16:19:38     matches!(hl_arc.lock().kind, HotLocationKind :: Compiled(_))

One for @ptersilie ?

@Pavel-Durov
Copy link
Contributor Author

Oh, that's new :)

@ptersilie
Copy link
Contributor

ptersilie commented Oct 17, 2023

16:19:38 thread '<unnamed>' panicked at ykrt/src/mt.rs:469:21:
16:19:38 assertion failed: sidetrace.is_none() ||
16:19:38     matches!(hl_arc.lock().kind, HotLocationKind :: Compiled(_))

One for @ptersilie ?

Hm, this error is caused by https://github.com/ykjit/yk/blob/master/ykrt/src/mt.rs#L470. This essentially says "if the current location doesn't have a compiled trace, then there also can't be a side trace". Since the only way sidetrace can be set here is via StopSideTracing (https://github.com/ykjit/yk/blob/master/ykrt/src/mt.rs#L279), I don't quite see how we can end up here.

It can't be a coincidence that this is caused by adding coroutines.lua. Maybe @ltratt is more suited for this?

@Pavel-Durov
Copy link
Contributor Author

Closing this PR as coroutine.lua test is currently failing.

@ltratt
Copy link
Contributor

ltratt commented Oct 18, 2023

@Pavel-Durov I don't think we should give up quite that quickly :) Can you investigate what part of the assert is failing? Might be worth inserting something like db!hl_arc.lock().kind); just before the assert.

@ltratt ltratt reopened this Oct 18, 2023
@Pavel-Durov
Copy link
Contributor Author

I'm not giving up :)
I'm just closing the PR until we fix the issue #58

@ltratt
Copy link
Contributor

ltratt commented Oct 18, 2023

The bug in that issues seems different than the failing assert here?

@Pavel-Durov
Copy link
Contributor Author

It does look different but I can't reproduce the same error as in buildbot with the latest YK version c6a0b9ca333408693a0c904bd502b1cdf0dc8d3f

@ltratt
Copy link
Contributor

ltratt commented Oct 18, 2023

I wonder if you might have a slightly stale build or similar. If you rm -rf target in yk and make clean in yklua, and rebuild both, does that change anything?

@Pavel-Durov
Copy link
Contributor Author

I will try doing clean build and run this test again

@Pavel-Durov
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 19, 2023
@bors
Copy link
Contributor

bors bot commented Oct 19, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@vext01
Copy link
Contributor

vext01 commented Oct 20, 2023

Is this one ready for squashing then? If so, please go ahead.

@Pavel-Durov
Copy link
Contributor Author

not ready yet, tests are failing again.

@Pavel-Durov
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 20, 2023
@bors
Copy link
Contributor

bors bot commented Oct 20, 2023

try

Build failed:

@Pavel-Durov
Copy link
Contributor Author

closing in favour of #79

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.

4 participants