Skip to content

docker pull inherits stderr and stdout, so docker logs are printed #2339

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

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

nuke-web3
Copy link

Motivation

Partial fix for #2338 .

Solution

https://doc.rust-lang.org/std/process/struct.Command.html#method.status

stdout and stderr are inherited from the parent.

PR Checklist

  • Added Tests N/A
  • Added Documentation N/A
  • Breaking changes N/A

(lmk if anything else is needed 🙏)

@nuke-web3 nuke-web3 changed the title docker pull inherited stderr and stdout, so logs are exposed to user docker pull inherits stderr and stdout, so docker logs are printed Jun 24, 2025
@nuke-web3 nuke-web3 marked this pull request as draft June 24, 2025 21:13
@nuke-web3
Copy link
Author

Well it seems this is not helpful for the original issue, although might expose the cuda docker pull logs that would be nice (yet to test though)

Digging deeper into the actual issue, it seems like we already should print the docker run command (that internally pulls the required image) with piping:

pub(crate) fn execute_command(mut command: Command, docker: bool) -> Result<()> {
// Add necessary tags for stdout and stderr from the command.
let mut child = command
.stdout(Stdio::piped())
.stderr(Stdio::piped())
.spawn()
.context("failed to spawn command")?;
let stdout = BufReader::new(child.stdout.take().unwrap());
let stderr = BufReader::new(child.stderr.take().unwrap());
// Add prefix to the output of the process depending on the context.
let msg = match docker {
true => "[sp1] [docker] ",
false => "[sp1] ",
};
// Pipe stdout and stderr to the parent process with [docker] prefix
let stdout_handle = thread::spawn(move || {
stdout.lines().for_each(|line| {
println!("{} {}", msg, line.unwrap());
});
});
stderr.lines().for_each(|line| {
eprintln!("{} {}", msg, line.unwrap());
});
stdout_handle.join().unwrap();
// Wait for the child process to finish and check the result.
let result = child.wait()?;
if !result.success() {
// Error message is already printed by cargo.
exit(result.code().unwrap_or(1))
}
Ok(())
}

but here I think progress bars etc. are not captured via piped, you need https://doc.rust-lang.org/std/process/struct.Stdio.html#method.inherit to get that I think, but then wouldn't be able to prefix I think... 🤔

indicates we might need separate logic to check the if image to use if not pulled, and pull it if so (that we can inherit to show progress I think)

@wangwa791
Copy link

Good

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.

2 participants