Skip to content

Commit

Permalink
Fix exec_replace
Browse files Browse the repository at this point in the history
  • Loading branch information
MrDenkoV committed Sep 23, 2023
1 parent d5c0df8 commit 52b2a92
Show file tree
Hide file tree
Showing 9 changed files with 109 additions and 47 deletions.
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ typed-builder = "0.15.1"
url = { version = "2.4.1", features = ["serde"] }
walkdir = "2.4.0"
which = "4.4.0"
windows-sys = "0.48.0"
xshell = "0.2.5"
xxhash-rust = { version = "0.8.7", features = ["xxh3"] }
zip = { version = "0.6.6", default-features = false, features = ["deflate"] }
Expand Down
1 change: 1 addition & 0 deletions scarb/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ typed-builder.workspace = true
url.workspace = true
walkdir.workspace = true
which.workspace = true
windows-sys.workspace = true
xxhash-rust.workspace = true
zip.workspace = true

Expand Down
2 changes: 0 additions & 2 deletions scarb/src/ops/subcommands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ pub fn execute_external_subcommand(
};

// TODO(mkaput): Jobserver.
// TODO(#129): Write a test that CTRL+C kills everything, like Cargo's death,
// but perhaps use an external bash script? Use Job Objects or smth else to fix it.

let mut cmd = Command::new(cmd);
cmd.args(args);
Expand Down
68 changes: 52 additions & 16 deletions scarb/src/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use scarb_ui::components::{Spinner, Status};

use crate::core::Config;

// TODO(#125): Do what is documented here, take a look at what cargo-util does.
/// Replaces the current process with the target process.
///
/// On Unix, this executes the process using the Unix syscall `execvp`, which will block this
Expand All @@ -30,21 +29,58 @@ use crate::core::Config;
/// itself later exits.
#[tracing::instrument(level = "debug")]
pub fn exec_replace(cmd: &mut Command) -> Result<()> {
let exit_status = cmd
.spawn()
.with_context(|| format!("failed to spawn: {}", cmd.get_program().to_string_lossy()))?
.wait()
.with_context(|| {
format!(
"failed to wait for process to finish: {}",
cmd.get_program().to_string_lossy()
)
})?;

if exit_status.success() {
Ok(())
} else {
bail!("process did not exit successfully: {exit_status}");
imp::exec_replace(cmd)
}

#[cfg(unix)]
mod imp {
use anyhow::{bail, Result};
use std::os::unix::process::CommandExt;
use std::process::Command;

pub fn exec_replace(cmd: &mut Command) -> Result<()> {
let err = cmd.exec();
bail!("process did not exit successfully: {err}")
}
}

#[cfg(windows)]
mod imp {
use anyhow::{bail, Context, Result};
use std::process::Command;
use windows_sys::Win32::Foundation::{BOOL, FALSE, TRUE};
use windows_sys::Win32::System::Console::SetConsoleCtrlHandler;

unsafe extern "system" fn ctrlc_handler(_: u32) -> BOOL {
// Do nothing; let the child process handle it.
TRUE
}

pub fn exec_replace(cmd: &mut Command) -> Result<()> {
unsafe {
if SetConsoleCtrlHandler(Some(ctrlc_handler), TRUE) == FALSE {
panic!("Could not set Ctrl-C handler.");
}
}

// Just execute the process as normal.

let exit_status = cmd
.spawn()
.with_context(|| format!("failed to spawn: {}", cmd.get_program().to_string_lossy()))?
.wait()
.with_context(|| {
format!(
"failed to wait for process to finish: {}",
cmd.get_program().to_string_lossy()
)
})?;

if exit_status.success() {
Ok(())
} else {
bail!("process did not exit successfully: {exit_status}");
}
}
}

Expand Down
37 changes: 8 additions & 29 deletions scarb/tests/subcommand.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use std::io::Read;
use std::net::TcpListener;
use std::process::{Child, Stdio};
use std::process::{Child, Command};
use std::{env, io};

use assert_fs::TempDir;
use indoc::{formatdoc, indoc};
#[cfg(unix)]
use indoc::indoc;
use scarb_test_support::cargo::cargo_bin;

use scarb_test_support::command::Scarb;
use scarb_test_support::filesystem::{path_with_temp_dir, write_script, write_simple_hello_script};
Expand Down Expand Up @@ -128,38 +130,15 @@ fn env_scarb_log_is_passed_verbatim() {
.success();
}

// TODO(#129): Fix this test.
#[test]
#[ignore] // something doesn't work here
fn ctrl_c_kills_everyone() {
let t = assert_fs::TempDir::new().unwrap();
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
let addr = listener.local_addr().unwrap().to_string();

write_script(
"hang-on-tcp",
&{
let addr = listener.local_addr().unwrap();
let ip = addr.ip();
let port = addr.port();
formatdoc!(
r#"
#!/usr/bin/env python3
import socket
sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
sock.connect(("{ip}", {port}))
sock.recv(10)
raise Exception("recv should never return")
"#
)
},
&t,
);

let mut child = Scarb::new()
.std()
let mut child = Command::new(cargo_bin("scarb-test-support"))
.arg("hang-on-tcp")
.env("PATH", path_with_temp_dir(&t))
.stdin(Stdio::piped())
.arg("--address")
.arg(addr)
.spawn()
.unwrap();

Expand Down
1 change: 1 addition & 0 deletions utils/scarb-test-support/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ publish = false
anyhow.workspace = true
assert_fs.workspace = true
camino.workspace = true
clap.workspace = true
dunce.workspace = true
indoc.workspace = true
itertools.workspace = true
Expand Down
38 changes: 38 additions & 0 deletions utils/scarb-test-support/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
use anyhow::Result;
use clap::{Parser, Subcommand};

#[derive(Parser, Clone, Debug)]
struct Args {
/// Subcommand and its arguments.
#[command(subcommand)]
pub command: Command,
}

#[derive(Subcommand, Clone, Debug)]
pub enum Command {
HangOnTcp(HangOnTcpArgs),
}

#[derive(Parser, Clone, Debug)]
pub struct HangOnTcpArgs {
#[arg(short, long)]
address: String,
}

fn main() -> Result<()> {
let args: Args = Args::parse();
match args.command {
Command::HangOnTcp(args) => hang_on_tcp(args),
}
}

fn hang_on_tcp(args: HangOnTcpArgs) -> Result<()> {
use std::io::Read;
use std::net::TcpStream;

let address: &str = args.address.as_ref();

let mut socket = TcpStream::connect(address).unwrap();
let _ = socket.read(&mut [0; 10]);
unreachable!("that read should never return");
}
6 changes: 6 additions & 0 deletions utils/scarb-test-support/tests/hack.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// HACK: We need integration tests, for cargo test to generate the binary "scarb-test-support",
// necessary for cargo_bin("scarb-test-support") to work correctly
// (used in ctrl_c_kills_everyone test in scarb/tests/subcommand.rs).

#[test]
fn binary_dependencies_hack() {}

0 comments on commit 52b2a92

Please sign in to comment.