Skip to content

Commit

Permalink
Bazel client: reduce dependency on POSIX API
Browse files Browse the repository at this point in the history
We can now compile blaze_util_windows.cc with
MSVC, yay! (when building //src:bazel
--cpu=x64_windows_msvc -k).

There are a lot of #ifdef's and TODOs so this
is a modest victory for now.

In this change:

- change blaze::MakeDirectories to return bool
instead of int, since that's how it was used
anyway, and to expect the permission mask as
unsigned int instead of mode_t, since the
former is good enough and compatible with
mode_t on POSIX while mode_t is not defined on
Windows

- move blaze::MakeDirectories into
blaze_util_<platform>

- implement envvar-handling in
blaze_util_<platform> and use it everywhere

See #2107

--
MOS_MIGRATED_REVID=139887503
  • Loading branch information
laszlocsomor authored and dslomov committed Nov 22, 2016
1 parent 124d55d commit cefa9a2
Show file tree
Hide file tree
Showing 12 changed files with 343 additions and 158 deletions.
34 changes: 17 additions & 17 deletions src/main/cpp/blaze.cc
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,7 @@ static void StartServerAndConnect(BlazeServer *server) {

// The server dir has the socket, so we don't allow access by other
// users.
if (MakeDirectories(server_dir, 0700) == -1) {
if (!MakeDirectories(server_dir, 0700)) {
pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
"server directory '%s' could not be created", server_dir.c_str());
}
Expand Down Expand Up @@ -797,7 +797,7 @@ class ExtractBlazeZipProcessor : public devtools_ijar::ZipExtractorProcessor {
virtual void Process(const char *filename, const devtools_ijar::u4 attr,
const devtools_ijar::u1 *data, const size_t size) {
string path = blaze_util::JoinPath(embedded_binaries_, filename);
if (MakeDirectories(blaze_util::Dirname(path), 0777) == -1) {
if (!MakeDirectories(blaze_util::Dirname(path), 0777)) {
pdie(blaze_exit_code::INTERNAL_ERROR,
"couldn't create '%s'", path.c_str());
}
Expand All @@ -818,7 +818,7 @@ class ExtractBlazeZipProcessor : public devtools_ijar::ZipExtractorProcessor {
static void ActuallyExtractData(const string &argv0,
const string &embedded_binaries) {
ExtractBlazeZipProcessor processor(embedded_binaries);
if (MakeDirectories(embedded_binaries, 0777) == -1) {
if (!MakeDirectories(embedded_binaries, 0777)) {

This comment has been minimized.

Copy link
@abergmeier-dsfishlabs

abergmeier-dsfishlabs Nov 24, 2016

Contributor

Is there a reason why MakeDirectories is not returning something a bit more descriptive like std::error_condition?

This comment has been minimized.

Copy link
@laszlocsomor

laszlocsomor Nov 24, 2016

Author Contributor

Not really, other than me having not seen the need for it and not being aware of std::error_condition :)
Do you think there's a need for more diverse error handling reporting here?

This comment has been minimized.

Copy link
@abergmeier-dsfishlabs

abergmeier-dsfishlabs Nov 25, 2016

Contributor

Especially with MakeDirectories it would be nice IMO to have an error message which at least roughly points into the direction of the problem. Having to look at all possible variants why a directory could not be created is tedious. Currently have exactly that problem.

This comment has been minimized.

Copy link
@laszlocsomor

laszlocsomor Nov 30, 2016

Author Contributor

Yeah that makes sense. Filed #2149

pdie(blaze_exit_code::INTERNAL_ERROR, "couldn't create '%s'",
embedded_binaries.c_str());
}
Expand Down Expand Up @@ -1254,7 +1254,7 @@ static void ComputeBaseDirectories(const string &self_path) {

const char *output_base = globals->options->output_base.c_str();
if (!blaze_util::PathExists(globals->options->output_base)) {
if (MakeDirectories(globals->options->output_base, 0777) == -1) {
if (!MakeDirectories(globals->options->output_base, 0777)) {
pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
"Output base directory '%s' could not be created",
output_base);
Expand All @@ -1280,32 +1280,32 @@ static void ComputeBaseDirectories(const string &self_path) {
}

static void CheckEnvironment() {
if (getenv("http_proxy") != NULL) {
if (!blaze::GetEnv("http_proxy").empty()) {
fprintf(stderr, "Warning: ignoring http_proxy in environment.\n");
unsetenv("http_proxy");
blaze::UnsetEnv("http_proxy");
}

if (getenv("LD_ASSUME_KERNEL") != NULL) {
if (!blaze::GetEnv("LD_ASSUME_KERNEL").empty()) {
// Fix for bug: if ulimit -s and LD_ASSUME_KERNEL are both
// specified, the JVM fails to create threads. See thread_stack_regtest.
// This is also provoked by LD_LIBRARY_PATH=/usr/lib/debug,
// or anything else that causes the JVM to use LinuxThreads.
fprintf(stderr, "Warning: ignoring LD_ASSUME_KERNEL in environment.\n");
unsetenv("LD_ASSUME_KERNEL");
blaze::UnsetEnv("LD_ASSUME_KERNEL");
}

if (getenv("LD_PRELOAD") != NULL) {
if (!blaze::GetEnv("LD_PRELOAD").empty()) {
fprintf(stderr, "Warning: ignoring LD_PRELOAD in environment.\n");
unsetenv("LD_PRELOAD");
blaze::UnsetEnv("LD_PRELOAD");
}

if (getenv("_JAVA_OPTIONS") != NULL) {
if (!blaze::GetEnv("_JAVA_OPTIONS").empty()) {
// This would override --host_jvm_args
fprintf(stderr, "Warning: ignoring _JAVA_OPTIONS in environment.\n");
unsetenv("_JAVA_OPTIONS");
blaze::UnsetEnv("_JAVA_OPTIONS");
}

if (getenv("TEST_TMPDIR") != NULL) {
if (!blaze::GetEnv("TEST_TMPDIR").empty()) {
fprintf(stderr, "INFO: $TEST_TMPDIR defined: output root default is "
"'%s'.\n", globals->options->output_root.c_str());
}
Expand All @@ -1316,10 +1316,10 @@ static void CheckEnvironment() {
// Make the JVM use ISO-8859-1 for parsing its command line because "blaze
// run" doesn't handle non-ASCII command line arguments. This is apparently
// the most reliable way to select the platform default encoding.
setenv("LANG", "en_US.ISO-8859-1", 1);
setenv("LANGUAGE", "en_US.ISO-8859-1", 1);
setenv("LC_ALL", "en_US.ISO-8859-1", 1);
setenv("LC_CTYPE", "en_US.ISO-8859-1", 1);
blaze::SetEnv("LANG", "en_US.ISO-8859-1");
blaze::SetEnv("LANGUAGE", "en_US.ISO-8859-1");
blaze::SetEnv("LC_ALL", "en_US.ISO-8859-1");
blaze::SetEnv("LC_CTYPE", "en_US.ISO-8859-1");
}

static void SetupStreams() {
Expand Down
126 changes: 16 additions & 110 deletions src/main/cpp/blaze_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@ const char kServerPidFile[] = "server.pid.txt";
const char kServerPidSymlink[] = "server.pid";

string GetUserName() {
const char *user = getenv("USER");
if (user && user[0] != '\0') return user;
string user = GetEnv("USER");
if (!user.empty()) {
return user;
}
errno = 0;
passwd *pwent = getpwuid(getuid()); // NOLINT (single-threaded)
if (pwent == NULL || pwent->pw_name == NULL) {
Expand All @@ -75,100 +77,6 @@ string MakeAbsolute(const string &path) {
return cwd + separator + path;
}

// Runs "stat" on `path`. Returns -1 and sets errno if stat fails or
// `path` isn't a directory. If check_perms is true, this will also
// make sure that `path` is owned by the current user and has `mode`
// permissions (observing the umask). It attempts to run chmod to
// correct the mode if necessary. If `path` is a symlink, this will
// check ownership of the link, not the underlying directory.
static int GetDirectoryStat(const string& path, mode_t mode, bool check_perms) {
struct stat filestat = {};
if (stat(path.c_str(), &filestat) == -1) {
return -1;
}

if (!S_ISDIR(filestat.st_mode)) {
errno = ENOTDIR;
return -1;
}

if (check_perms) {
// If this is a symlink, run checks on the link. (If we did lstat above
// then it would return false for ISDIR).
struct stat linkstat = {};
if (lstat(path.c_str(), &linkstat) != 0) {
return -1;
}
if (linkstat.st_uid != geteuid()) {
// The directory isn't owned by me.
errno = EACCES;
return -1;
}

mode_t mask = umask(022);
umask(mask);
mode = (mode & ~mask);
if ((filestat.st_mode & 0777) != mode
&& chmod(path.c_str(), mode) == -1) {
// errno set by chmod.
return -1;
}
}
return 0;
}

static int MakeDirectories(const string& path, mode_t mode, bool childmost) {
if (path.empty() || path == "/") {
errno = EACCES;
return -1;
}

int retval = GetDirectoryStat(path, mode, childmost);
if (retval == 0) {
return 0;
}

if (errno == ENOENT) {
// Path does not exist, attempt to create its parents, then it.
string parent = blaze_util::Dirname(path);
if (MakeDirectories(parent, mode, false) == -1) {
// errno set by stat.
return -1;
}

if (mkdir(path.c_str(), mode) == -1) {
if (errno == EEXIST) {
if (childmost) {
// If there are multiple bazel calls at the same time then the
// directory could be created between the MakeDirectories and mkdir
// calls. This is okay, but we still have to check the permissions.
return GetDirectoryStat(path, mode, childmost);
} else {
// If this isn't the childmost directory, we don't care what the
// permissions were. If it's not even a directory then that error will
// get caught when we attempt to create the next directory down the
// chain.
return 0;
}
}
// errno set by mkdir.
return -1;
}
return 0;
}

return retval;
}

// mkdir -p path. Returns 0 if the path was created or already exists and could
// be chmod-ed to exactly the given permissions. If final part of the path is a
// symlink, this ensures that the destination of the symlink has the desired
// permissions. It also checks that the directory or symlink is owned by us.
// On failure, this returns -1 and sets errno.
int MakeDirectories(const string& path, mode_t mode) {
return MakeDirectories(path, mode, true);
}

// Replaces 'contents' with contents of 'fd' file descriptor.
// If `max_size` is positive, the method reads at most that many bytes; if it
// is 0, the method reads the whole file.
Expand All @@ -185,7 +93,7 @@ bool ReadFileDescriptor(int fd, string *content, size_t max_size) {
}
content->append(buf, r);
if (max_size > 0) {
if (max_size > r) {
if (max_size > static_cast<size_t>(r)) {
max_size -= r;
} else {
break;
Expand Down Expand Up @@ -235,24 +143,24 @@ bool UnlinkPath(const string &file_path) {
}

bool IsEmacsTerminal() {
string emacs = getenv("EMACS") == nullptr ? "" : getenv("EMACS");
string inside_emacs =
getenv("INSIDE_EMACS") == nullptr ? "" : getenv("INSIDE_EMACS");
string emacs = GetEnv("EMACS");
string inside_emacs = GetEnv("INSIDE_EMACS");
// GNU Emacs <25.1 (and ~all non-GNU emacsen) set EMACS=t, but >=25.1 doesn't
// do that and instead sets INSIDE_EMACS=<stuff> (where <stuff> can look like
// e.g. "25.1.1,comint"). So we check both variables for maximum
// compatibility.
return emacs == "t" || inside_emacs != "";
return emacs == "t" || !inside_emacs.empty();
}

// Returns true iff both stdout and stderr are connected to a
// terminal, and it can support color and cursor movement
// (this is computed heuristically based on the values of
// environment variables).
bool IsStandardTerminal() {
string term = getenv("TERM") == nullptr ? "" : getenv("TERM");
if (term == "" || term == "dumb" || term == "emacs" || term == "xterm-mono" ||
term == "symbolics" || term == "9term" || IsEmacsTerminal()) {
string term = GetEnv("TERM");
if (term.empty() || term == "dumb" || term == "emacs" ||
term == "xterm-mono" || term == "symbolics" || term == "9term" ||
IsEmacsTerminal()) {
return false;
}
return isatty(STDOUT_FILENO) && isatty(STDERR_FILENO);
Expand All @@ -265,10 +173,10 @@ int GetTerminalColumns() {
if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws) != -1) {
return ws.ws_col;
}
const char* columns_env = getenv("COLUMNS");
if (columns_env != NULL && columns_env[0] != '\0') {
string columns_env = GetEnv("COLUMNS");
if (!columns_env.empty()) {
char* endptr;
int columns = blaze_util::strto32(columns_env, &endptr, 10);
int columns = blaze_util::strto32(columns_env.c_str(), &endptr, 10);
if (*endptr == '\0') { // $COLUMNS is a valid number
return columns;
}
Expand Down Expand Up @@ -305,9 +213,7 @@ bool GetNullaryOption(const char *arg, const char *key) {
return true;
}

bool VerboseLogging() {
return getenv("VERBOSE_BLAZE_CLIENT") != NULL;
}
bool VerboseLogging() { return !GetEnv("VERBOSE_BLAZE_CLIENT").empty(); }

// Read the Jvm version from a file descriptor. The read fd
// should contains a similar output as the java -version output.
Expand Down
4 changes: 0 additions & 4 deletions src/main/cpp/blaze_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@ std::string GetUserName();
// MakeAbsolute("C:/foo") ---> "C:/foo"
std::string MakeAbsolute(const std::string &path);

// mkdir -p path. All newly created directories use the given mode.
// Returns -1 on failure, sets errno.
int MakeDirectories(const std::string &path, mode_t mode);

// Replaces 'content' with contents of file 'filename'.
// If `max_size` is positive, the method reads at most that many bytes; if it
// is 0, the method reads the whole file.
Expand Down
6 changes: 3 additions & 3 deletions src/main/cpp/blaze_util_darwin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,9 @@ bool IsSharedLibrary(const string &filename) {
}

string GetDefaultHostJavabase() {
const char *java_home = getenv("JAVA_HOME");
if (java_home) {
return std::string(java_home);
string java_home = GetEnv("JAVA_HOME");
if (!java_home.empty()) {
return java_home;
}

FILE *output = popen("/usr/libexec/java_home -v 1.7+", "r");
Expand Down
7 changes: 2 additions & 5 deletions src/main/cpp/blaze_util_freebsd.cc
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,8 @@ bool IsSharedLibrary(const string &filename) {

string GetDefaultHostJavabase() {
// if JAVA_HOME is defined, then use it as default.
const char *javahome = getenv("JAVA_HOME");
if (javahome != NULL) {
return string(javahome);
}
return "/usr/local/openjdk8";
string javahome = getenv("JAVA_HOME");
return !javahome.empty() ? javahome : "/usr/local/openjdk8";
}

void WriteSystemSpecificProcessIdentifier(const string& server_dir) {
Expand Down
11 changes: 11 additions & 0 deletions src/main/cpp/blaze_util_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,17 @@ std::string GetHashedBaseDir(const std::string& root,
// user, and not accessible to anyone else.
void CreateSecureOutputRoot(const std::string& path);

// mkdir -p path. All newly created directories use the given mode.
// `mode` should be an octal permission mask, e.g. 0755
// Returns false on failure, sets errno.
bool MakeDirectories(const std::string &path, unsigned int mode);

std::string GetEnv(const std::string& name);

void SetEnv(const std::string& name, const std::string& value);

void UnsetEnv(const std::string& name);

} // namespace blaze

#endif // BAZEL_SRC_MAIN_CPP_BLAZE_UTIL_PLATFORM_H_
Loading

0 comments on commit cefa9a2

Please sign in to comment.