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

Docs for child processes #8486

Merged
merged 5 commits into from
Apr 13, 2021
Merged

Docs for child processes #8486

merged 5 commits into from
Apr 13, 2021

Conversation

psteckler
Copy link
Member

Add a README for Child_processes.

Notes:

  • there's a mention of the process kind for Libp2p_helper, which was added to master-qa, but not compatible, yet. This mention will become valid in the near future, then.
  • there are some changes of coda to mina, but not for environment variables, which still in use in the codebase
  • the exit 99 when the prover or verifier dies is changed to a failwith. I ran a demo node locally, killed the prover, and verified the daemon still exits. Reviewer: Coda_run.handle_crash gets called in this case, is that OK?

@psteckler psteckler requested a review from a team as a code owner March 30, 2021 22:51
@psteckler psteckler added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Mar 30, 2021
@psteckler psteckler changed the base branch from develop to compatible March 30, 2021 23:01
Copy link
Member

@nholland94 nholland94 left a comment

Choose a reason for hiding this comment

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

I think the docs could use some more details here to be in line with our new documentation guidelines. In general, I'd prefer if we don't make code changes along with the documentation PRs; I'm not totally sure these code changes are benign.

src/lib/child_processes/README.md Show resolved Hide resolved
src/lib/child_processes/README.md Outdated Show resolved Hide resolved
src/lib/child_processes/child_processes.ml Show resolved Hide resolved
src/lib/child_processes/child_processes.ml Outdated Show resolved Hide resolved
src/lib/child_processes/termination.ml Show resolved Hide resolved
@psteckler
Copy link
Member Author

In general, I'd prefer if we don't make code changes along with the documentation PRs

I'll move those changes to a separate PR, if after reading my justifications, you're still not convinced.

@nholland94
Copy link
Member

In general, I'd prefer if we don't make code changes along with the documentation PRs

I'll move those changes to a separate PR, if after reading my justifications, you're still not convinced.

Sorry for the delayed response. It's ok to leave them here, the justifications work for me.

@psteckler psteckler added the ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR label Apr 13, 2021
@mrmr1993 mrmr1993 merged commit 99fb795 into compatible Apr 13, 2021
@mrmr1993 mrmr1993 deleted the feature/child-process-docs branch April 13, 2021 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants