Skip to content

Commit 8879a03

Browse files
bradkingdigit-google
bradking
and
digit-google
committed
jobserver: enable pipe-based protocol for GNU make 4.3 and below
Restore code from an earlier draft of the jobserver pull-request before support for the pipe-based protocol was disabled: * ninja-build#2506 (comment) * https://github.com/digit-google/ninja/releases/tag/JOBSERVER-with-pipe-support Co-authored-by: David 'Digit' Turner <[email protected]>
1 parent b4d51f6 commit 8879a03

7 files changed

Lines changed: 92 additions & 56 deletions

File tree

‎doc/manual.asciidoc‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -207,19 +207,18 @@ This feature is automatically enabled under the following conditions:
207207
208208
- The `MAKEFLAGS` environment variable is defined and describes a valid
209209
jobserver mode using `--jobserver-auth=SEMAPHORE_NAME` on Windows, or
210-
`--jobserver-auth=fifo:PATH` on Posix.
210+
`--jobserver-auth=fifo:PATH`, `--jobserver-auth=<read>,<write>`, or
211+
even `--jobserver-fds=R,W` on Posix.
211212
212213
In this case, Ninja will use the jobserver pool of job slots to control
213214
parallelism, instead of its default parallel implementation.
214215
215216
Note that load-average limitations (i.e. when using `-l<count>`)
216217
are still being enforced in this mode.
217218
218-
IMPORTANT: On Posix, only the FIFO-based version of the protocol, which is
219-
implemented by GNU Make 4.4 and higher, is supported. Ninja will detect
220-
when a pipe-based jobserver is being used (i.e. when `MAKEFLAGS` contains
221-
`--jobserver-auth=<read>,<write>`) and will print a warning, but will
222-
otherwise ignore it.
219+
IMPORTANT: On Posix, the FIFO-based version of the protocol, which is
220+
implemented by GNU Make 4.4 and higher, is preferred. Ninja will warn
221+
when a pipe-based jobserver is being used.
223222
224223
Environment variables
225224
~~~~~~~~~~~~~~~~~~~~~

‎misc/jobserver_test.py‎

Lines changed: 1 addition & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -253,42 +253,7 @@ def test_jobserver_client_with_posix_fifo(self):
253253

254254
@unittest.skipIf(_PLATFORM_IS_WINDOWS, "These test methods do not work on Windows")
255255
def test_jobserver_client_with_posix_pipe(self):
256-
# Verify that setting up a --pipe server does not make Ninja exit with an error.
257-
# Instead, a warning is printed.
258-
task_count = 4
259-
build_plan = generate_build_plan(task_count)
260-
with BuildDir(build_plan) as b:
261-
262-
prefix_args = [
263-
sys.executable,
264-
"-S",
265-
_JOBSERVER_POOL_SCRIPT,
266-
"--pipe",
267-
f"--jobs={task_count}",
268-
]
269-
270-
def run_ninja_with_jobserver_pipe(args):
271-
ret = b.ninja_spawn(args, prefix_args=prefix_args)
272-
ret.check_returncode()
273-
return ret.stdout, ret.stderr
274-
275-
output, error = run_ninja_with_jobserver_pipe(["all"])
276-
if _DEBUG:
277-
print(f"OUTPUT [{output}]\nERROR [{error}]\n", file=sys.stderr)
278-
self.assertTrue(error.find("Pipe-based protocol is not supported!") >= 0)
279-
280-
max_overlaps = compute_max_overlapped_spans(b.path, task_count)
281-
self.assertEqual(max_overlaps, task_count)
282-
283-
# Using an explicit -j<N> ignores the jobserver pool.
284-
b.ninja_clean()
285-
output, error = run_ninja_with_jobserver_pipe(["-j1", "all"])
286-
if _DEBUG:
287-
print(f"OUTPUT [{output}]\nERROR [{error}]\n", file=sys.stderr)
288-
self.assertFalse(error.find("Pipe-based protocol is not supported!") >= 0)
289-
290-
max_overlaps = compute_max_overlapped_spans(b.path, task_count)
291-
self.assertEqual(max_overlaps, 1)
256+
self._run_client_test([sys.executable, "-S", _JOBSERVER_POOL_SCRIPT, "--pipe"])
292257

293258
def _test_MAKEFLAGS_value(
294259
self, ninja_args: T.List[str] = [], prefix_args: T.List[str] = []

‎src/jobserver-posix.cc‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,41 @@ bool IsFifoDescriptor(int fd) {
3333
return (ret == 0) && ((info.st_mode & S_IFMT) == S_IFIFO);
3434
}
3535

36+
bool SetNonBlockingFd(int fd) {
37+
int flags = fcntl(fd, F_GETFL, 0);
38+
if (!(flags & O_NONBLOCK)) {
39+
int ret = fcntl(fd, F_SETFL, flags | O_NONBLOCK);
40+
if (ret < 0)
41+
return false;
42+
}
43+
return true;
44+
}
45+
46+
bool SetCloseOnExecFd(int fd) {
47+
int flags = fcntl(fd, F_GETFD, 0);
48+
if (!(flags & FD_CLOEXEC)) {
49+
int ret = fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
50+
if (ret < 0)
51+
return false;
52+
}
53+
return true;
54+
}
55+
56+
// Duplicate the descriptor and make the result non-blocking and
57+
// close-on-exec.
58+
bool DuplicateDescriptor(int from_fd, int* to_fd) {
59+
int new_fd = dup(from_fd);
60+
if (new_fd < 0) {
61+
return false;
62+
}
63+
if (!SetNonBlockingFd(new_fd) || !SetCloseOnExecFd(new_fd)) {
64+
::close(new_fd);
65+
return false;
66+
}
67+
*to_fd = new_fd;
68+
return true;
69+
}
70+
3671
// Implementation of Jobserver::Client for Posix systems
3772
class PosixJobserverClient : public Jobserver::Client {
3873
public:
@@ -77,6 +112,28 @@ class PosixJobserverClient : public Jobserver::Client {
77112
(void)ret; // Nothing can be done in case of error here.
78113
}
79114

115+
// Initialize instance with two explicit pipe file descriptors.
116+
bool InitWithPipeFds(int read_fd, int write_fd, std::string* error) {
117+
// Verify that the file descriptors belong to FIFOs.
118+
if (!IsFifoDescriptor(read_fd) || !IsFifoDescriptor(write_fd)) {
119+
*error = "Invalid file descriptors";
120+
return false;
121+
}
122+
// Duplicate the file descriptors to make then non-blocking, and
123+
// close-on-exec. This is important because the original descriptors
124+
// might be inherited by sub-processes of this client.
125+
if (!DuplicateDescriptor(read_fd, &read_fd_)) {
126+
*error = "Could not duplicate read descriptor";
127+
return false;
128+
}
129+
if (!DuplicateDescriptor(write_fd, &write_fd_)) {
130+
*error = "Could not duplicate write descriptor";
131+
// Let destructor close read_fd_.
132+
return false;
133+
}
134+
return true;
135+
}
136+
80137
// Initialize with FIFO file path.
81138
bool InitWithFifo(const std::string& fifo_path, std::string* error) {
82139
if (fifo_path.empty()) {
@@ -122,6 +179,8 @@ std::unique_ptr<Jobserver::Client> Jobserver::Client::Create(
122179
auto client = std::unique_ptr<PosixJobserverClient>(new PosixJobserverClient);
123180
if (config.mode == Jobserver::Config::kModePosixFifo) {
124181
success = client->InitWithFifo(config.path, error);
182+
} else if (config.mode == Jobserver::Config::kModePipe) {
183+
success = client->InitWithPipeFds(config.read_fd, config.write_fd, error);
125184
} else {
126185
*error = "Unsupported jobserver mode";
127186
}

‎src/jobserver.cc‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,16 @@ bool GetPrefixedValue(StringPiece input, StringPiece prefix,
3737

3838
// Try to read a comma-separated pair of file descriptors from |input|.
3939
// On success return true and set |config->mode| accordingly. Otherwise return
40-
// false if the input doesn't follow the appropriate format. Note that the
41-
// values are not saved since pipe mode is not supported.
40+
// false if the input doesn't follow the appropriate format.
4241
bool GetFileDescriptorPair(StringPiece input, Jobserver::Config* config) {
43-
int read_fd = 1, write_fd = -1;
4442
std::string pair = input.AsString();
45-
if (sscanf(pair.c_str(), "%d,%d", &read_fd, &write_fd) != 2)
43+
if (sscanf(pair.c_str(), "%d,%d", &config->read_fd, &config->write_fd) != 2)
4644
return false;
4745

4846
// From
4947
// https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html Any
5048
// negative descriptor means the feature is disabled.
51-
if (read_fd < 0 || write_fd < 0)
49+
if (config->read_fd < 0 || config->write_fd < 0)
5250
config->mode = Jobserver::Config::kModeNone;
5351
else
5452
config->mode = Jobserver::Config::kModePipe;
@@ -189,15 +187,15 @@ bool Jobserver::ParseNativeMakeFlagsValue(const char* makeflags_env,
189187
if (!ParseMakeFlagsValue(makeflags_env, config, error))
190188
return false;
191189

192-
if (config->mode == Jobserver::Config::kModePipe) {
193-
*error = "Pipe-based protocol is not supported!";
194-
return false;
195-
}
196190
#ifdef _WIN32
197191
if (config->mode == Jobserver::Config::kModePosixFifo) {
198192
*error = "FIFO mode is not supported on Windows!";
199193
return false;
200194
}
195+
if (config->mode == Jobserver::Config::kModePipe) {
196+
*error = "Pipe mode is not supported on Windows!";
197+
return false;
198+
}
201199
#else // !_WIN32
202200
if (config->mode == Jobserver::Config::kModeWin32Semaphore) {
203201
*error = "Semaphore mode is not supported on Posix!";

‎src/jobserver.h‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,7 @@ struct Jobserver {
111111
/// kModePipe means that `--jobserver-auth=R,W` is used to
112112
/// pass a pair of file descriptors to client processes. This also
113113
/// matches `--jobserver-fds=R,W` which is an old undocumented
114-
/// variant of the same scheme. This mode is not supported by
115-
/// Ninja, but recognized by the parser.
114+
/// variant of the same scheme.
116115
///
117116
/// kModePosixFifo means that `--jobserver-auth=fifo:PATH` is used to
118117
/// pass the path of a Posix FIFO to client processes. This is not
@@ -145,6 +144,11 @@ struct Jobserver {
145144
/// For kModeSemaphore, this is the name of the Win32 semaphore to use.
146145
std::string path;
147146

147+
/// For kModePipe, these are the file descriptor values
148+
/// extracted from MAKEFLAGS.
149+
int read_fd = -1;
150+
int write_fd = -1;
151+
148152
/// Return true if this instance matches an active implementation mode.
149153
/// This does not try to validate configuration parameters though.
150154
bool HasMode() { return mode != kModeNone; }

‎src/jobserver_test.cc‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,8 @@ TEST(Jobserver, ParseMakeFlagsValue) {
151151
ASSERT_TRUE(Jobserver::ParseMakeFlagsValue("--jobserver-auth=10,42", &config,
152152
&error));
153153
EXPECT_EQ(Jobserver::Config::kModePipe, config.mode);
154+
EXPECT_EQ(10, config.read_fd);
155+
EXPECT_EQ(42, config.write_fd);
154156

155157
config = {};
156158
error.clear();
@@ -187,9 +189,11 @@ TEST(Jobserver, ParseNativeMakeFlagsValue) {
187189
// --jobserver-auth=R,W is not supported.
188190
config = {};
189191
error.clear();
190-
EXPECT_FALSE(Jobserver::ParseNativeMakeFlagsValue("--jobserver-auth=3,4",
191-
&config, &error));
192-
EXPECT_EQ(error, "Pipe-based protocol is not supported!");
192+
EXPECT_TRUE(Jobserver::ParseNativeMakeFlagsValue("--jobserver-auth=3,4",
193+
&config, &error));
194+
EXPECT_EQ(Jobserver::Config::kModePipe, config.mode);
195+
EXPECT_EQ(3, config.read_fd);
196+
EXPECT_EQ(4, config.write_fd);
193197

194198
#ifdef _WIN32
195199
// --jobserver-auth=NAME works on Windows.

‎src/ninja.cc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1578,6 +1578,13 @@ std::unique_ptr<Jobserver::Client> NinjaMain::SetupJobserverClient(
15781578

15791579
if (config_.verbosity > BuildConfig::NO_STATUS_UPDATE) {
15801580
status->Info("Jobserver mode detected: %s", makeflags);
1581+
#ifndef _WIN32
1582+
if (jobserver_config.mode == Jobserver::Config::kModePipe) {
1583+
status->Warning(
1584+
"Jobserver 'pipe' mode detected, a pool that implements 'fifo' mode "
1585+
"would be more reliable!");
1586+
}
1587+
#endif
15811588
}
15821589

15831590
result = Jobserver::Client::Create(jobserver_config, &err);

0 commit comments

Comments
 (0)