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

Support --jobserver-auth=fifo:PATH #49

Merged
merged 2 commits into from
Feb 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,29 @@ jobs:
- name: Install Rust (rustup)
run: rustup update ${{ matrix.rust }} --no-self-update && rustup default ${{ matrix.rust }}
shell: bash

- run: cargo test

# Compile it from source (temporarily)
- name: Make GNU Make from source
if: ${{ !startsWith(matrix.os, 'windows') }}
env:
VERSION: "4.4.1"
shell: bash
run: |
wget -q "https://ftp.gnu.org/gnu/make/make-${VERSION}.tar.gz"
tar zxf "make-${VERSION}.tar.gz"
pushd "make-${VERSION}"
./configure
make
popd
cp -rp "make-${VERSION}/make" .
- name: Test against GNU Make from source
if: ${{ !startsWith(matrix.os, 'windows') }}
shell: bash
run:
MAKE="${PWD}/make" cargo test

rustfmt:
name: Rustfmt
runs-on: ubuntu-latest
Expand Down
5 changes: 4 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
//! The jobserver implementation can be found in [detail online][docs] but
//! basically boils down to a cross-process semaphore. On Unix this is
//! implemented with the `pipe` syscall and read/write ends of a pipe and on
//! Windows this is implemented literally with IPC semaphores.
//! Windows this is implemented literally with IPC semaphores. Starting from
//! GNU `make` version 4.4, named pipe becomes the default way in communication
//! on Unix. This crate also supports that feature in the sense of inheriting
//! and forwarding the correct environment.
//!
//! The jobserver protocol in `make` also dictates when tokens are acquired to
//! run child work, and clients using this crate should take care to implement
Expand Down
86 changes: 71 additions & 15 deletions src/unix.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
use libc::c_int;

use std::fs::File;
use std::fs::{File, OpenOptions};
use std::io::{self, Read, Write};
use std::mem;
use std::mem::MaybeUninit;
use std::os::unix::prelude::*;
use std::path::{Path, PathBuf};
use std::process::Command;
use std::ptr;
use std::sync::{Arc, Once};
use std::thread::{self, Builder, JoinHandle};
use std::time::Duration;

#[derive(Debug)]
pub struct Client {
read: File,
write: File,
pub enum Client {
/// `--jobserver-auth=R,W`
Pipe { read: File, write: File },
/// `--jobserver-auth=fifo:PATH`
Fifo { file: File, path: PathBuf },
}

#[derive(Debug)]
Expand All @@ -30,16 +33,18 @@ impl Client {
// wrong!
const BUFFER: [u8; 128] = [b'|'; 128];

set_nonblocking(client.write.as_raw_fd(), true)?;
let mut write = client.write();

set_nonblocking(write.as_raw_fd(), true)?;

while limit > 0 {
let n = limit.min(BUFFER.len());

(&client.write).write_all(&BUFFER[..n])?;
write.write_all(&BUFFER[..n])?;
limit -= n;
}

set_nonblocking(client.write.as_raw_fd(), false)?;
set_nonblocking(write.as_raw_fd(), false)?;

Ok(client)
}
Expand Down Expand Up @@ -77,6 +82,31 @@ impl Client {
}

pub unsafe fn open(s: &str) -> Option<Client> {
Client::from_fifo(s).or_else(|| Client::from_pipe(s))
}

/// `--jobserver-auth=fifo:PATH`
fn from_fifo(s: &str) -> Option<Client> {
let mut parts = s.splitn(2, ':');
if parts.next().unwrap() != "fifo" {
return None;
}
let path = match parts.next() {
Some(p) => Path::new(p),
None => return None,
};
let file = match OpenOptions::new().read(true).write(true).open(path) {
Ok(f) => f,
Err(_) => return None,
};
Some(Client::Fifo {
file,
path: path.into(),
})
}

/// `--jobserver-auth=R,W`
unsafe fn from_pipe(s: &str) -> Option<Client> {
let mut parts = s.splitn(2, ',');
let read = parts.next().unwrap();
let write = match parts.next() {
Expand Down Expand Up @@ -110,12 +140,28 @@ impl Client {
}

unsafe fn from_fds(read: c_int, write: c_int) -> Client {
Client {
Client::Pipe {
read: File::from_raw_fd(read),
write: File::from_raw_fd(write),
}
}

/// Gets the read end of our jobserver client.
fn read(&self) -> &File {
match self {
Client::Pipe { read, .. } => read,
Client::Fifo { file, .. } => file,
}
}

/// Gets the write end of our jobserver client.
fn write(&self) -> &File {
match self {
Client::Pipe { write, .. } => write,
Client::Fifo { file, .. } => file,
}
}

pub fn acquire(&self) -> io::Result<Acquired> {
// Ignore interrupts and keep trying if that happens
loop {
Expand Down Expand Up @@ -150,11 +196,12 @@ impl Client {
// to shut us down, so we otherwise punt all errors upwards.
unsafe {
let mut fd: libc::pollfd = mem::zeroed();
fd.fd = self.read.as_raw_fd();
let mut read = self.read();
fd.fd = read.as_raw_fd();
fd.events = libc::POLLIN;
loop {
let mut buf = [0];
match (&self.read).read(&mut buf) {
match read.read(&mut buf) {
Ok(1) => return Ok(Some(Acquired { byte: buf[0] })),
Ok(_) => {
return Err(io::Error::new(
Expand Down Expand Up @@ -192,7 +239,7 @@ impl Client {
// always quickly release a token). If that turns out to not be the
// case we'll get an error anyway!
let byte = data.map(|d| d.byte).unwrap_or(b'+');
match (&self.write).write(&[byte])? {
match self.write().write(&[byte])? {
1 => Ok(()),
_ => Err(io::Error::new(
io::ErrorKind::Other,
Expand All @@ -202,22 +249,31 @@ impl Client {
}

pub fn string_arg(&self) -> String {
format!("{},{}", self.read.as_raw_fd(), self.write.as_raw_fd())
match self {
Client::Pipe { read, write } => format!("{},{}", read.as_raw_fd(), write.as_raw_fd()),
Client::Fifo { path, .. } => format!("fifo:{}", path.to_str().unwrap()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a bad idea to use the new fifo: syntax by default since this is a new syntax and not every program supports this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. That's why we only support this syntax when inheriting from the environment, and it's really in a fifo style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. That's why we only support this syntax when inheriting from the environment, and it's really in a fifo style.

Yeah this makes sense, though IMO it would be better to keep using the old syntax.

I do appreciate this new syntax since it means we no longer have to register a callback to Command::pre_exec, which blocks the use of vfork.

}
}

pub fn available(&self) -> io::Result<usize> {
let mut len = MaybeUninit::<c_int>::uninit();
cvt(unsafe { libc::ioctl(self.read.as_raw_fd(), libc::FIONREAD, len.as_mut_ptr()) })?;
cvt(unsafe { libc::ioctl(self.read().as_raw_fd(), libc::FIONREAD, len.as_mut_ptr()) })?;
Ok(unsafe { len.assume_init() } as usize)
}

pub fn configure(&self, cmd: &mut Command) {
match self {
// We `File::open`ed it when inheriting from environment,
// so no need to set cloexec for fifo.
Client::Fifo { .. } => return,
Client::Pipe { .. } => {}
};
// Here we basically just want to say that in the child process
// we'll configure the read/write file descriptors to *not* be
// cloexec, so they're inherited across the exec and specified as
// integers through `string_arg` above.
let read = self.read.as_raw_fd();
let write = self.write.as_raw_fd();
let read = self.read().as_raw_fd();
let write = self.write().as_raw_fd();
unsafe {
cmd.pre_exec(move || {
set_cloexec(read, false)?;
Expand Down