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

fix: last --jobserver-auth wins #67

Merged
merged 5 commits into from
Feb 5, 2024
Merged
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
75 changes: 67 additions & 8 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,16 +279,10 @@ impl Client {
}
};

let (arg, pos) = match ["--jobserver-fds=", "--jobserver-auth="]
.iter()
.map(|&arg| var.find(arg).map(|pos| (arg, pos)))
.find_map(|pos| pos)
{
Some((arg, pos)) => (arg, pos),
let s = match find_jobserver_auth(var) {
Some(s) => s,
None => return FromEnv::new_err(FromEnvErrorInner::NoJobserver, env, var_os),
};

let s = var[pos + arg.len()..].split(' ').next().unwrap();
match imp::Client::open(s, check_pipe) {
Ok(c) => FromEnv::new_ok(Client { inner: Arc::new(c) }, env, var_os),
Err(err) => FromEnv::new_err(err, env, var_os),
Expand Down Expand Up @@ -588,9 +582,74 @@ impl HelperState {
}
}

/// Finds and returns the value of `--jobserver-auth=<VALUE>` in the given
/// environment variable.
///
/// Precedence rules:
///
/// * The last instance wins [^1].
/// * `--jobserver-fds=` as a fallback when no `--jobserver-auth=` is present [^2].
///
/// [^1]: See ["GNU `make` manual: Sharing Job Slots with GNU `make`"](https://www.gnu.org/software/make/manual/make.html#Job-Slots)
/// _"Be aware that the `MAKEFLAGS` variable may contain multiple instances of
/// the `--jobserver-auth=` option. Only the last instance is relevant."_
///
/// [^2]: Refer to [the release announcement](https://git.savannah.gnu.org/cgit/make.git/tree/NEWS?h=4.2#n31)
/// of GNU Make 4.2, which states that `--jobserver-fds` was initially an
/// internal-only flag and was later renamed to `--jobserver-auth`.
fn find_jobserver_auth(var: &str) -> Option<&str> {
["--jobserver-auth=", "--jobserver-fds="]
.iter()
.find_map(|&arg| var.rsplit_once(arg).map(|(_, s)| s))
.and_then(|s| s.split(' ').next())
}

#[test]
fn no_helper_deadlock() {
let x = crate::Client::new(32).unwrap();
let _y = x.clone();
std::mem::drop(x.into_helper_thread(|_| {}).unwrap());
}

#[test]
fn test_find_jobserver_auth() {
let cases = [
("", None),
("-j2", None),
("-j2 --jobserver-auth=3,4", Some("3,4")),
("--jobserver-auth=3,4 -j2", Some("3,4")),
("--jobserver-auth=3,4", Some("3,4")),
("--jobserver-auth=fifo:/myfifo", Some("fifo:/myfifo")),
("--jobserver-auth=", Some("")),
("--jobserver-auth", None),
("--jobserver-fds=3,4", Some("3,4")),
("--jobserver-fds=fifo:/myfifo", Some("fifo:/myfifo")),
("--jobserver-fds=", Some("")),
("--jobserver-fds", None),
(
"--jobserver-auth=auth-a --jobserver-auth=auth-b",
Some("auth-b"),
),
(
"--jobserver-auth=auth-b --jobserver-auth=auth-a",
Some("auth-a"),
),
("--jobserver-fds=fds-a --jobserver-fds=fds-b", Some("fds-b")),
("--jobserver-fds=fds-b --jobserver-fds=fds-a", Some("fds-a")),
(
"--jobserver-auth=auth-a --jobserver-fds=fds-a --jobserver-auth=auth-b",
Some("auth-b"),
),
(
"--jobserver-fds=fds-a --jobserver-auth=auth-a --jobserver-fds=fds-b",
Some("auth-a"),
),
];
for (var, expected) in cases {
let actual = find_jobserver_auth(var);
assert_eq!(
actual, expected,
"expect {expected:?}, got {actual:?}, input `{var:?}`"
);
}
}