From 97da63efcd4e3b927d437e4fd560b8fd0b38585f Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 31 Jan 2024 16:35:18 -0500 Subject: [PATCH 1/5] refactor: extract `--jobserver-auth=` to a free function --- src/lib.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 3091aea..7ac618e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -279,11 +279,7 @@ impl Client { } }; - let (arg, pos) = match ["--jobserver-fds=", "--jobserver-auth="] - .iter() - .map(|&arg| var.find(arg).map(|pos| (arg, pos))) - .find_map(|pos| pos) - { + let (arg, pos) = match find_jobserver_auth(var) { Some((arg, pos)) => (arg, pos), None => return FromEnv::new_err(FromEnvErrorInner::NoJobserver, env, var_os), }; @@ -588,6 +584,13 @@ impl HelperState { } } +fn find_jobserver_auth(var: &str) -> Option<(&str, usize)> { + ["--jobserver-fds=", "--jobserver-auth="] + .iter() + .map(|&arg| var.find(arg).map(|pos| (arg, pos))) + .find_map(|pos| pos) +} + #[test] fn no_helper_deadlock() { let x = crate::Client::new(32).unwrap(); From 0084ef3d5798fd02bd5022e98fde6bf4f2f6f612 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 1 Feb 2024 17:10:23 -0500 Subject: [PATCH 2/5] refactor: `find_jobserver_auth` returns a slice --- src/lib.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 7ac618e..4d6d1d5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -279,12 +279,10 @@ impl Client { } }; - let (arg, pos) = match find_jobserver_auth(var) { - 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), @@ -584,11 +582,11 @@ impl HelperState { } } -fn find_jobserver_auth(var: &str) -> Option<(&str, usize)> { +fn find_jobserver_auth(var: &str) -> Option<&str> { ["--jobserver-fds=", "--jobserver-auth="] .iter() - .map(|&arg| var.find(arg).map(|pos| (arg, pos))) - .find_map(|pos| pos) + .find_map(|&arg| var.split_once(arg).map(|(_, s)| s)) + .and_then(|s| s.split(' ').next()) } #[test] From 1575c784c2932661aa7d4e7decb85927d30bc96c Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 31 Jan 2024 16:36:28 -0500 Subject: [PATCH 3/5] test: demonstrate the current precedence of finding `--jobserver-auth` --- src/lib.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 4d6d1d5..a2ab494 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -595,3 +595,28 @@ fn no_helper_deadlock() { let _y = x.clone(); std::mem::drop(x.into_helper_thread(|_| {}).unwrap()); } + +#[test] +fn test_find_jobserver_auth() { + let cases = [ + ("--jobserver-auth=auth-a --jobserver-auth=auth-b", "auth-a"), + ("--jobserver-auth=auth-b --jobserver-auth=auth-a", "auth-b"), + ("--jobserver-fds=fds-a --jobserver-fds=fds-b", "fds-a"), + ("--jobserver-fds=fds-b --jobserver-fds=fds-a", "fds-b"), + ( + "--jobserver-auth=auth-a --jobserver-fds=fds-a --jobserver-auth=auth-b", + "fds-a", + ), + ( + "--jobserver-fds=fds-a --jobserver-auth=auth-a --jobserver-fds=fds-b", + "fds-a", + ), + ]; + for (var, expected) in cases { + let actual = find_jobserver_auth(var).unwrap(); + assert_eq!( + actual, expected, + "expect {expected:?}, got {actual:?}, input `{var:?}`" + ); + } +} From 68a7f8cd2dd9f22ada57c21602451747510ef13d Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 31 Jan 2024 16:40:58 -0500 Subject: [PATCH 4/5] fix: last `--jobserver-auth` wins From the GNU make manual[^1]: > Be aware that the `MAKEFLAGS` variable may contain multiple > instances of the `--jobserver-auth=` option. > Only the last instance is relevant. Hence this commit makes it so. With this commit, `--jobserver-auth` also takes precedence over `--jobserver-fds`, even when `--jobserver-fds` is the last instance of these flags. This is made intentionally since `--jobserver-fds` was an undocumented internal-only flag before `--jobserver-auth` made public, according to this announcement[^2]. [^1]: https://www.gnu.org/software/make/manual/make.html#Job-Slots [^2]: https://git.savannah.gnu.org/cgit/make.git/tree/NEWS?h=4.2#n31 --- src/lib.rs | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index a2ab494..2af0e85 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -582,10 +582,25 @@ impl HelperState { } } +/// Finds and returns the value of `--jobserver-auth=` 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-fds=", "--jobserver-auth="] + ["--jobserver-auth=", "--jobserver-fds="] .iter() - .find_map(|&arg| var.split_once(arg).map(|(_, s)| s)) + .find_map(|&arg| var.rsplit_once(arg).map(|(_, s)| s)) .and_then(|s| s.split(' ').next()) } @@ -599,17 +614,17 @@ fn no_helper_deadlock() { #[test] fn test_find_jobserver_auth() { let cases = [ - ("--jobserver-auth=auth-a --jobserver-auth=auth-b", "auth-a"), - ("--jobserver-auth=auth-b --jobserver-auth=auth-a", "auth-b"), - ("--jobserver-fds=fds-a --jobserver-fds=fds-b", "fds-a"), - ("--jobserver-fds=fds-b --jobserver-fds=fds-a", "fds-b"), + ("--jobserver-auth=auth-a --jobserver-auth=auth-b", "auth-b"), + ("--jobserver-auth=auth-b --jobserver-auth=auth-a", "auth-a"), + ("--jobserver-fds=fds-a --jobserver-fds=fds-b", "fds-b"), + ("--jobserver-fds=fds-b --jobserver-fds=fds-a", "fds-a"), ( "--jobserver-auth=auth-a --jobserver-fds=fds-a --jobserver-auth=auth-b", - "fds-a", + "auth-b", ), ( "--jobserver-fds=fds-a --jobserver-auth=auth-a --jobserver-fds=fds-b", - "fds-a", + "auth-a", ), ]; for (var, expected) in cases { From 171d33d826e42d159814aa7eaf9d2a39778fda52 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sat, 3 Feb 2024 08:22:50 -0500 Subject: [PATCH 5/5] test: more tests for `find_jobserver_auth` --- src/lib.rs | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 2af0e85..fe000c3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -614,21 +614,39 @@ fn no_helper_deadlock() { #[test] fn test_find_jobserver_auth() { let cases = [ - ("--jobserver-auth=auth-a --jobserver-auth=auth-b", "auth-b"), - ("--jobserver-auth=auth-b --jobserver-auth=auth-a", "auth-a"), - ("--jobserver-fds=fds-a --jobserver-fds=fds-b", "fds-b"), - ("--jobserver-fds=fds-b --jobserver-fds=fds-a", "fds-a"), + ("", 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", - "auth-b", + Some("auth-b"), ), ( "--jobserver-fds=fds-a --jobserver-auth=auth-a --jobserver-fds=fds-b", - "auth-a", + Some("auth-a"), ), ]; for (var, expected) in cases { - let actual = find_jobserver_auth(var).unwrap(); + let actual = find_jobserver_auth(var); assert_eq!( actual, expected, "expect {expected:?}, got {actual:?}, input `{var:?}`"