Skip to content

Commit

Permalink
Prepare PR for merging - part II
Browse files Browse the repository at this point in the history
- remove unnecessary "struct" from TokenPool
- add PAPCFUNC cast to QueryUserAPC()
- remove hard-coded MAKEFLAGS string from win32
- remove useless build test CompleteNoWork
- rename TokenPoolTest to TestTokenPool
- add tokenpool modules to CMake build
- remove unused no-op TokenPool implementation
- address review comments from

ninja-build#1140 (review)
ninja-build#1140 (review)
ninja-build#1140 (comment)
ninja-build#1140 (comment)
  • Loading branch information
stefanb2 authored and bradking committed May 25, 2021
1 parent 77ad35a commit 2aa885c
Show file tree
Hide file tree
Showing 14 changed files with 90 additions and 114 deletions.
8 changes: 7 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ add_library(libninja OBJECT
src/parser.cc
src/state.cc
src/string_piece_util.cc
src/tokenpool-gnu-make.cc
src/util.cc
src/version.cc
)
Expand All @@ -104,12 +105,16 @@ if(WIN32)
src/msvc_helper-win32.cc
src/msvc_helper_main-win32.cc
src/getopt.c
src/tokenpool-gnu-make-win32.cc
)
if(MSVC)
target_sources(libninja PRIVATE src/minidump-win32.cc)
endif()
else()
target_sources(libninja PRIVATE src/subprocess-posix.cc)
target_sources(libninja PRIVATE
src/subprocess-posix.cc
src/tokenpool-gnu-make-posix.cc
)
if(CMAKE_SYSTEM_NAME STREQUAL "OS400" OR CMAKE_SYSTEM_NAME STREQUAL "AIX")
target_sources(libninja PRIVATE src/getopt.c)
endif()
Expand Down Expand Up @@ -182,6 +187,7 @@ if(BUILD_TESTING)
src/string_piece_util_test.cc
src/subprocess_test.cc
src/test.cc
src/tokenpool_test.cc
src/util_test.cc
)
if(WIN32)
Expand Down
2 changes: 1 addition & 1 deletion src/build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,7 @@ struct RealCommandRunner : public CommandRunner {
// copy of config_.max_load_average; can be modified by TokenPool setup
double max_load_average_;
SubprocessSet subprocs_;
TokenPool *tokens_;
TokenPool* tokens_;
map<const Subprocess*, Edge*> subproc_to_edge_;
};

Expand Down
10 changes: 0 additions & 10 deletions src/build_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3461,16 +3461,6 @@ void BuildTokenTest::EnqueueBooleans(vector<bool>& booleans, int count, va_list
}
}

TEST_F(BuildTokenTest, CompleteNoWork) {
// plan should not execute anything
string err;

EXPECT_TRUE(builder_.Build(&err));
EXPECT_EQ("", err);

EXPECT_EQ(0u, token_command_runner_.commands_ran_.size());
}

TEST_F(BuildTokenTest, DoNotAquireToken) {
// plan should execute one command
string err;
Expand Down
4 changes: 2 additions & 2 deletions src/subprocess-posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ Subprocess *SubprocessSet::Add(const string& command, bool use_console) {
}

#ifdef USE_PPOLL
bool SubprocessSet::DoWork(struct TokenPool* tokens) {
bool SubprocessSet::DoWork(TokenPool* tokens) {
vector<pollfd> fds;
nfds_t nfds = 0;

Expand Down Expand Up @@ -315,7 +315,7 @@ bool SubprocessSet::DoWork(struct TokenPool* tokens) {
}

#else // !defined(USE_PPOLL)
bool SubprocessSet::DoWork(struct TokenPool* tokens) {
bool SubprocessSet::DoWork(TokenPool* tokens) {
fd_set set;
int nfds = 0;
FD_ZERO(&set);
Expand Down
2 changes: 1 addition & 1 deletion src/subprocess-win32.cc
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ Subprocess *SubprocessSet::Add(const string& command, bool use_console) {
return subprocess;
}

bool SubprocessSet::DoWork(struct TokenPool* tokens) {
bool SubprocessSet::DoWork(TokenPool* tokens) {
DWORD bytes_read;
Subprocess* subproc;
OVERLAPPED* overlapped;
Expand Down
2 changes: 1 addition & 1 deletion src/subprocess.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ struct SubprocessSet {
~SubprocessSet();

Subprocess* Add(const std::string& command, bool use_console = false);
bool DoWork(struct TokenPool* tokens);
bool DoWork(TokenPool* tokens);
Subprocess* NextFinished();
void Clear();

Expand Down
24 changes: 12 additions & 12 deletions src/subprocess_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const char* kSimpleCommand = "cmd /c dir \\";
const char* kSimpleCommand = "ls /";
#endif

struct TokenPoolTest : public TokenPool {
struct TestTokenPool : public TokenPool {
bool Acquire() { return false; }
void Reserve() {}
void Release() {}
Expand All @@ -58,7 +58,7 @@ struct TokenPoolTest : public TokenPool {

struct SubprocessTest : public testing::Test {
SubprocessSet subprocs_;
TokenPoolTest tokens_;
TestTokenPool tokens_;
};

} // anonymous namespace
Expand All @@ -73,7 +73,7 @@ TEST_F(SubprocessTest, BadCommandStderr) {
// Pretend we discovered that stderr was ready for writing.
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());

EXPECT_EQ(ExitFailure, subproc->Finish());
EXPECT_NE("", subproc->GetOutput());
Expand All @@ -89,7 +89,7 @@ TEST_F(SubprocessTest, NoSuchCommand) {
// Pretend we discovered that stderr was ready for writing.
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());

EXPECT_EQ(ExitFailure, subproc->Finish());
EXPECT_NE("", subproc->GetOutput());
Expand All @@ -109,7 +109,7 @@ TEST_F(SubprocessTest, InterruptChild) {
while (!subproc->Done()) {
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());

EXPECT_EQ(ExitInterrupted, subproc->Finish());
}
Expand All @@ -135,7 +135,7 @@ TEST_F(SubprocessTest, InterruptChildWithSigTerm) {
while (!subproc->Done()) {
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());

EXPECT_EQ(ExitInterrupted, subproc->Finish());
}
Expand All @@ -161,7 +161,7 @@ TEST_F(SubprocessTest, InterruptChildWithSigHup) {
while (!subproc->Done()) {
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());

EXPECT_EQ(ExitInterrupted, subproc->Finish());
}
Expand Down Expand Up @@ -190,7 +190,7 @@ TEST_F(SubprocessTest, Console) {
while (!subproc->Done()) {
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());

EXPECT_EQ(ExitSuccess, subproc->Finish());
}
Expand All @@ -206,7 +206,7 @@ TEST_F(SubprocessTest, SetWithSingle) {
while (!subproc->Done()) {
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());
ASSERT_EQ(ExitSuccess, subproc->Finish());
ASSERT_NE("", subproc->GetOutput());

Expand Down Expand Up @@ -243,7 +243,7 @@ TEST_F(SubprocessTest, SetWithMulti) {
ASSERT_GT(subprocs_.running_.size(), 0u);
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());
ASSERT_EQ(0u, subprocs_.running_.size());
ASSERT_EQ(3u, subprocs_.finished_.size());

Expand Down Expand Up @@ -278,7 +278,7 @@ TEST_F(SubprocessTest, SetWithLots) {
subprocs_.ResetTokenAvailable();
while (!subprocs_.running_.empty())
subprocs_.DoWork(NULL);
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());
for (size_t i = 0; i < procs.size(); ++i) {
ASSERT_EQ(ExitSuccess, procs[i]->Finish());
ASSERT_NE("", procs[i]->GetOutput());
Expand All @@ -298,7 +298,7 @@ TEST_F(SubprocessTest, ReadStdin) {
while (!subproc->Done()) {
subprocs_.DoWork(NULL);
}
ASSERT_EQ(false, subprocs_.IsTokenAvailable());
ASSERT_FALSE(subprocs_.IsTokenAvailable());
ASSERT_EQ(ExitSuccess, subproc->Finish());
ASSERT_EQ(1u, subprocs_.finished_.size());
}
Expand Down
17 changes: 8 additions & 9 deletions src/tokenpool-gnu-make-posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ struct GNUmakeTokenPoolPosix : public GNUmakeTokenPool {

virtual int GetMonitorFd();

virtual const char *GetEnv(const char *name) { return getenv(name); };
virtual bool ParseAuth(const char *jobserver);
virtual const char* GetEnv(const char* name) { return getenv(name); };
virtual bool ParseAuth(const char* jobserver);
virtual bool AcquireToken();
virtual bool ReturnToken();

Expand Down Expand Up @@ -82,14 +82,13 @@ bool GNUmakeTokenPoolPosix::SetAlarmHandler() {
act.sa_handler = CloseDupRfd;
if (sigaction(SIGALRM, &act, &old_act_) < 0) {
perror("sigaction:");
return(false);
} else {
restore_ = true;
return(true);
return false;
}
restore_ = true;
return true;
}

bool GNUmakeTokenPoolPosix::ParseAuth(const char *jobserver) {
bool GNUmakeTokenPoolPosix::ParseAuth(const char* jobserver) {
int rfd = -1;
int wfd = -1;
if ((sscanf(jobserver, "%*[^=]=%d,%d", &rfd, &wfd) == 2) &&
Expand Down Expand Up @@ -195,9 +194,9 @@ bool GNUmakeTokenPoolPosix::ReturnToken() {
}

int GNUmakeTokenPoolPosix::GetMonitorFd() {
return(rfd_);
return rfd_;
}

struct TokenPool *TokenPool::Get() {
TokenPool* TokenPool::Get() {
return new GNUmakeTokenPoolPosix;
}
36 changes: 19 additions & 17 deletions src/tokenpool-gnu-make-win32.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@

#include "tokenpool-gnu-make.h"

// always include first to make sure other headers do the correct thing...
// Always include this first.
// Otherwise the other system headers don't work correctly under Win32
#include <windows.h>

#include <ctype.h>
Expand All @@ -32,8 +33,8 @@ struct GNUmakeTokenPoolWin32 : public GNUmakeTokenPool {
virtual void WaitForTokenAvailability(HANDLE ioport);
virtual bool TokenIsAvailable(ULONG_PTR key);

virtual const char *GetEnv(const char *name);
virtual bool ParseAuth(const char *jobserver);
virtual const char* GetEnv(const char* name);
virtual bool ParseAuth(const char* jobserver);
virtual bool AcquireToken();
virtual bool ReturnToken();

Expand Down Expand Up @@ -98,19 +99,19 @@ GNUmakeTokenPoolWin32::~GNUmakeTokenPoolWin32() {
}
}

const char *GNUmakeTokenPoolWin32::GetEnv(const char *name) {
const char* GNUmakeTokenPoolWin32::GetEnv(const char* name) {
// getenv() does not work correctly together with tokenpool_tests.cc
static char buffer[MAX_PATH + 1];
if (GetEnvironmentVariable("MAKEFLAGS", buffer, sizeof(buffer)) == 0)
if (GetEnvironmentVariable(name, buffer, sizeof(buffer)) == 0)
return NULL;
return(buffer);
return buffer;
}

bool GNUmakeTokenPoolWin32::ParseAuth(const char *jobserver) {
bool GNUmakeTokenPoolWin32::ParseAuth(const char* jobserver) {
// match "--jobserver-auth=gmake_semaphore_<INTEGER>..."
const char *start = strchr(jobserver, '=');
const char* start = strchr(jobserver, '=');
if (start) {
const char *end = start;
const char* end = start;
unsigned int len;
char c, *auth;

Expand All @@ -119,14 +120,15 @@ bool GNUmakeTokenPoolWin32::ParseAuth(const char *jobserver) {
break;
len = end - start; // includes string terminator in count

if ((len > 1) && ((auth = (char *)malloc(len)) != NULL)) {
if ((len > 1) && ((auth = (char*)malloc(len)) != NULL)) {
strncpy(auth, start + 1, len - 1);
auth[len - 1] = '\0';

if ((semaphore_jobserver_ = OpenSemaphore(SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */
FALSE, /* Child processes DON'T inherit */
auth /* Semaphore name */
)) != NULL) {
if ((semaphore_jobserver_ =
OpenSemaphore(SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */
FALSE, /* Child processes DON'T inherit */
auth /* Semaphore name */
)) != NULL) {
free(auth);
return true;
}
Expand Down Expand Up @@ -171,7 +173,7 @@ DWORD GNUmakeTokenPoolWin32::SemaphoreThread() {
}

DWORD WINAPI GNUmakeTokenPoolWin32::SemaphoreThreadWrapper(LPVOID param) {
GNUmakeTokenPoolWin32 *This = (GNUmakeTokenPoolWin32 *) param;
GNUmakeTokenPoolWin32* This = (GNUmakeTokenPoolWin32*) param;
return This->SemaphoreThread();
}

Expand Down Expand Up @@ -216,7 +218,7 @@ void GNUmakeTokenPoolWin32::WaitForTokenAvailability(HANDLE ioport) {

bool GNUmakeTokenPoolWin32::TokenIsAvailable(ULONG_PTR key) {
// alert child thread to break wait on token semaphore
QueueUserAPC(&NoopAPCFunc, child_, (ULONG_PTR)NULL);
QueueUserAPC((PAPCFUNC)&NoopAPCFunc, child_, (ULONG_PTR)NULL);

// return true when GetQueuedCompletionStatus() returned our key
return key == (ULONG_PTR) this;
Expand All @@ -232,6 +234,6 @@ void GNUmakeTokenPoolWin32::WaitForObject(HANDLE object) {
Win32Fatal("WaitForSingleObject");
}

struct TokenPool *TokenPool::Get() {
TokenPool* TokenPool::Get() {
return new GNUmakeTokenPoolWin32;
}
Loading

0 comments on commit 2aa885c

Please sign in to comment.