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

Don't leak channels. #1601

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Don't leak channels. #1601

merged 2 commits into from
Dec 12, 2024

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Apr 15, 2024

Alternative to #1578 and #1577.
This PR requires ppx_expect and alcotest to be changed.

Because the jsoo codebase rely on ppx_expect for its tests, we have a Chicken-and-egg situation.

I can make a minor release of jsoo that would include new runtime function while keeping the existing storage mechanism.
Once jsoo is released, ppx_expect can rely on the new runtime functions.
After ppx_expect is released, we can merge this PR.

@tkluck-janestreet, how does that sound ?

@TyOverby
Copy link
Collaborator

What are the required ppx_expect changes?

@hhugo
Copy link
Member Author

hhugo commented Apr 15, 2024

What are the required ppx_expect changes?

janestreet/ppx_expect#54

@hhugo hhugo mentioned this pull request Apr 15, 2024
@dkalinichenko-js
Copy link

dkalinichenko-js commented Oct 11, 2024

Hi, you should be able to merge this after updating ppx_expect to v0.17.1. (see ocaml/opam-repository#26695)

@hhugo
Copy link
Member Author

hhugo commented Oct 11, 2024

Hi, you should be able to merge this after updating ppx_expect to v0.17.1. (see ocaml/opam-repository#26695)

@dkalinichenko-js, I probably won't be able to upgrade to v0.17.1 because it will probably still be broken on windows. See janestreet/ppx_expect#57

@dkalinichenko-js
Copy link

I see. I'll submit another release of ppx_expect with the Windows fix Monday.

@hhugo
Copy link
Member Author

hhugo commented Oct 23, 2024

@dkalinichenko-js, the new blocker is that the new release of ppx_expect is not available for ocaml 4.14, preventing us to run our expect_tests with OCaml 4.14

@hhugo
Copy link
Member Author

hhugo commented Dec 10, 2024

@hhugo hhugo force-pushed the better-chan branch 2 times, most recently from c6a9a8f to b488cb8 Compare December 11, 2024 21:13
@hhugo hhugo removed the blocked label Dec 11, 2024
@hhugo hhugo merged commit a79fb93 into master Dec 12, 2024
0 of 27 checks passed
@hhugo hhugo deleted the better-chan branch December 12, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants